sketch-hq / nicene Goto Github PK

Group aliases and imports such that imports follow aliases.

This check asserts the whether "lib" in Path.split(source_file.filename) is true. However, this is finding cases like: test/lib/context/employees_test.exs, which is technically correct (and indicates that the test probably belongs in a different folder), but is not correct because the test is under the test/ folder.

It seems to me that the condition should be that a test file is, first and foremost, found outside of the test/ directory, or if that’s not preferred, that there’s a second check to make sure that the file’s path doesn’t begin with test/.

The link to documentation in both the README and on hex.pm result in a 404 Not Found on hexdocs.pm. Not sure if this is a hexdocs issue or a code issue resulting in documentation problems.

We have presently Ecto models in a big flat directory where it's hard to find the right file given several other application concerns are found in the same place. It would be useful to have a check in place that ensures they are placed in a set directory in the project hierarchy.

Ensure that test like modules under test have expected suffix to be detected and executed as tests.

We should have an automated CI build, it can easily be build using github actions

Guard against hyphens in sourcecode filenames (and advise using underscores instead?)

In order to provide rails for following our new guidelines on how to write Ecto queries, we should have a new Nicene.ComposableQueries check that needs to check for:

• using as to define aliases when joining schemas
• using |> from(as: :alias) at the start of a query (aka when a module is directly passed to where, join, select...) to alias that first schema
• Additionally, we can enforce the usage of the Ecto macro API by checking that we don't pass select:, where: or similar keywords to the from macro.

I'm not entirely sure how doable these are since these ecto functions are actually macros, but let's give it a try.

There is a match error when FileTopToBottom encounters a macro-constructed module.

I’ll be opening a draft PR with the following added to the FileTopToBottom test cases.

defmodule App.File.Macro do
  defmacro __using__(_) do
    quote do
      defmodule Module.safe_concat(__MODULE__, Test) do
        def test_2(), do: test_3()

        def test() do
          IO.inspect("IN HERE")
          test_2()
        end

        def test_2(), do: :ok
      end
    end
  end
end

defmodule App.File do
  use App.File.Macro
end

This results in

     ** (MatchError) no match of right hand side value: [{{:., [line: 8, column: 23], [{:__aliases__, [line: 8, column: 17], [:Module]}, :safe_concat]}, [line: 8, column: 23], [{:__MODULE__, [line: 8, column: 36], nil}, {:__aliases__, [line: 8, column: 48], [:Test]}]}, [do: {:__block__, [], [{:def, [line: 9, column: 9], [{:test_2, [line: 9, column: 13], []}, [do: {:test_3, [line: 9, column: 27], []}]]}, {:def, [line: 11, column: 9], [{:test, [line: 11, column: 13], []}, [do: {:__block__, [], [{{:., [line: 12, column: 13], [{:__aliases__, [line: 12, column: 11], [:IO]}, :inspect]}, [line: 12, column: 13], ["IN HERE"]}, {:test_2, [line: 13, column: 11], []}]}]]}, {:def, [line: 16, column: 9], [{:test_2, [line: 16, column: 13], []}, [do: :ok]]}]}]]

It'd be excellent to be able to enforce that Ecto models are documented to a minimal standard. For starters, requiring a moduledoc and associations to have a non-empty @desc would be a great start.

Ensure that GraphQL schema type definitions include without an exception a non-empty @desc.

Where @doc entries are included, they should contain at least some minimal character count of text.

Sometimes I have small helper modules in the same file as the module which uses them, and it seems that nicene does not take into account module boundaries.

With the code below, in each module the public functions are before private functions, so I would not expect any warnings.

Running mix credo on this with the default nicene 0.5.0 configuration produces this result:

Checking 3 source files ...

  Code Readability
┃
┃ [R] ↗ Private function is defined before a public function
┃       lib/nicene_report.ex:10 #(NiceneReport.result)

Please report incorrect results: https://github.com/rrrene/credo/issues

Analysis took 0.03 seconds (0.00s to load, 0.03s running 53 checks)
7 mods/funs, found 1 code readability issue.

defmodule NiceneReport do
  @moduledoc false

  alias NiceneReport.Result

  def hello do
    result()
  end

  defp result do
    Result.world()
  end

  defmodule Result do
    @moduledoc false

    def world do
      world_atom()
    end

    defp world_atom do
      :world
    end
  end
end

As per the DB query organisation rule set agreed, Ecto.Query should only be imported in X...X.Query modules.

I’m checking out Nicene for some of our code and I’m seeing some errors. The first error is preventing Credo from completing:

** (config) Error while running Elixir.Nicene.ConsistentFunctionDefinitions on test/support/fixtures/map_attribute.ex
08:14:08.208 [error] Task #PID<0.1043.0> started from #PID<0.93.0> terminating
** (Protocol.UndefinedError) protocol String.Chars not implemented for {:unquote, [line: 18, column: 11], [{:json_attrs, [line: 18, column: 19], nil}]} of type Tuple
    (elixir 1.10.4) lib/string/chars.ex:3: String.Chars.impl_for!/1
    (elixir 1.10.4) lib/string/chars.ex:22: String.Chars.to_string/1
    lib/consistent_function_definitions.ex:42: Nicene.ConsistentFunctionDefinitions.process_line/3
    (elixir 1.10.4) lib/enum.ex:2111: Enum."-reduce/3-lists^foldl/2-0-"/3
    lib/consistent_function_definitions.ex:19: Nicene.ConsistentFunctionDefinitions.run/2
    lib/credo/check/runner.ex:168: Credo.Check.Runner.do_run_check_on_single_source_file/3
    lib/credo/check/runner.ex:161: Credo.Check.Runner.run_check_on_single_source_file/3
    (elixir 1.10.4) lib/enum.ex:1396: Enum."-map/2-lists^map/1-0-"/2
Function: #Function<9.9572610/0 in Credo.Check.Runner.run/2>
    Args: []

I modified this to print out some details of the line:

      IO.inspect({
        Process.get(:source_file),
        line_no,
        line,
        Process.get(:position),
        funs[line_no]
      })

This is what I see:

{%SourceFile<test/support/fixtures/map_attribute.ex>, 18,
 "      def unquote(json_attrs)(merge \\\\ %{}) do", 1,
 {:unquote, [line: 18, column: 11],
  [{:json_attrs, [line: 18, column: 19], nil}]}}
{%SourceFile<lib/lowes_loyalty/models/tokens/macros.ex>, 191,
 "      def unquote(names.find)(%User{id: id}), do: unquote(names.find)(%{user_id: id})",
 1,
 {:unquote, [line: 191, column: 11],
  [
    {{:., [line: 191, column: 24],
      [{:names, [line: 191, column: 19], nil}, :find]},
     [no_parens: true, line: 191, column: 24], []}
  ]}}

So the problem here is in funs[line_no] not producing something that can be interpolated into the regex at line 40.

I’ll try to work a unit test that reproduces this, but it will take some time. (I’m seeing a similar, but different, error in FileTopToBottom. I will file a different report on that when I am clearer on what the exact issue is with that test.)

Elixir compiler triggers this warning:

warning: clauses with the same name and arity (number of arguments) should be grouped together

but says nothing if functions with the very same name but different arity are separated in the code. Credo does not seem to have a check for this either, but I think it's a nice-to-have.