Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compatibility with Credo 1.6? #22

Open
altdsoy opened this issue Feb 21, 2022 · 3 comments
Open

Compatibility with Credo 1.6? #22

altdsoy opened this issue Feb 21, 2022 · 3 comments
Labels
bug Something isn't working

Comments

@altdsoy
Copy link

altdsoy commented Feb 21, 2022

Hello,

I'm trying to add CredoNaming but nothing happens,
I guess it's not longer compatible with credo 1.6?

Everything is default Phoenix. Here is the only addition to mix.exs for example:

# mix.exs
  {:credo, "~> 1.6", only: [:dev, :test], runtime: false},
  {:credo_naming, "~> 1.0", only: [:dev, :test], runtime: false},

Here are my default .credo.exs file generated by mix credo gen.config.

# .credo.exs generated by mix credo gen.config
# This file contains the configuration for Credo and you are probably reading
# this after creating it with `mix credo.gen.config`.
#
# If you find anything wrong or unclear in this file, please report an
# issue on GitHub: https://github.com/rrrene/credo/issues
#
%{
  #
  # You can have as many configs as you like in the `configs:` field.
  configs: [
    %{
      #
      # Run any config using `mix credo -C <name>`. If no config name is given
      # "default" is used.
      #
      name: "default",
      #
      # These are the files included in the analysis:
      files: %{
        #
        # You can give explicit globs or simply directories.
        # In the latter case `**/*.{ex,exs}` will be used.
        #
        included: [
          "lib/",
          "src/",
          "test/",
          "web/",
          "apps/*/lib/",
          "apps/*/src/",
          "apps/*/test/",
          "apps/*/web/"
        ],
        excluded: [~r"/_build/", ~r"/deps/", ~r"/node_modules/"]
      },
      #
      # Load and configure plugins here:
      #
      plugins: [],
      #
      # If you create your own checks, you must specify the source files for
      # them here, so they can be loaded by Credo before running the analysis.
      #
      requires: [],
      #
      # If you want to enforce a style guide and need a more traditional linting
      # experience, you can change `strict` to `true` below:
      #
      strict: false,
      #
      # To modify the timeout for parsing files, change this value:
      #
      parse_timeout: 5000,
      #
      # If you want to use uncolored output by default, you can change `color`
      # to `false` below:
      #
      color: true,
      #
      # You can customize the parameters of any check by adding a second element
      # to the tuple.
      #
      # To disable a check put `false` as second element:
      #
      #     {Credo.Check.Design.DuplicatedCode, false}
      #
      checks: %{
        enabled: [
          #
          ## Consistency Checks
          #
          {Credo.Check.Consistency.ExceptionNames, []},
          {Credo.Check.Consistency.LineEndings, []},
          {Credo.Check.Consistency.ParameterPatternMatching, []},
          {Credo.Check.Consistency.SpaceAroundOperators, []},
          {Credo.Check.Consistency.SpaceInParentheses, []},
          {Credo.Check.Consistency.TabsOrSpaces, []},

          #
          ## Design Checks
          #
          # You can customize the priority of any check
          # Priority values are: `low, normal, high, higher`
          #
          {Credo.Check.Design.AliasUsage,
           [priority: :low, if_nested_deeper_than: 2, if_called_more_often_than: 0]},
          # You can also customize the exit_status of each check.
          # If you don't want TODO comments to cause `mix credo` to fail, just
          # set this value to 0 (zero).
          #
          {Credo.Check.Design.TagTODO, [exit_status: 2]},
          {Credo.Check.Design.TagFIXME, []},

          #
          ## Readability Checks
          #
          {Credo.Check.Readability.AliasOrder, []},
          {Credo.Check.Readability.FunctionNames, []},
          {Credo.Check.Readability.LargeNumbers, []},
          {Credo.Check.Readability.MaxLineLength, [priority: :low, max_length: 120]},
          {Credo.Check.Readability.ModuleAttributeNames, []},
          {Credo.Check.Readability.ModuleDoc, []},
          {Credo.Check.Readability.ModuleNames, []},
          {Credo.Check.Readability.ParenthesesInCondition, []},
          {Credo.Check.Readability.ParenthesesOnZeroArityDefs, []},
          {Credo.Check.Readability.PipeIntoAnonymousFunctions, []},
          {Credo.Check.Readability.PredicateFunctionNames, []},
          {Credo.Check.Readability.PreferImplicitTry, []},
          {Credo.Check.Readability.RedundantBlankLines, []},
          {Credo.Check.Readability.Semicolons, []},
          {Credo.Check.Readability.SpaceAfterCommas, []},
          {Credo.Check.Readability.StringSigils, []},
          {Credo.Check.Readability.TrailingBlankLine, []},
          {Credo.Check.Readability.TrailingWhiteSpace, []},
          {Credo.Check.Readability.UnnecessaryAliasExpansion, []},
          {Credo.Check.Readability.VariableNames, []},
          {Credo.Check.Readability.WithSingleClause, []},

          #
          ## Refactoring Opportunities
          #
          {Credo.Check.Refactor.Apply, []},
          {Credo.Check.Refactor.CondStatements, []},
          {Credo.Check.Refactor.CyclomaticComplexity, []},
          {Credo.Check.Refactor.FunctionArity, []},
          {Credo.Check.Refactor.LongQuoteBlocks, []},
          {Credo.Check.Refactor.MatchInCondition, []},
          {Credo.Check.Refactor.MapJoin, []},
          {Credo.Check.Refactor.NegatedConditionsInUnless, []},
          {Credo.Check.Refactor.NegatedConditionsWithElse, []},
          {Credo.Check.Refactor.Nesting, []},
          {Credo.Check.Refactor.UnlessWithElse, []},
          {Credo.Check.Refactor.WithClauses, []},
          {Credo.Check.Refactor.FilterFilter, []},
          {Credo.Check.Refactor.RejectReject, []},
          {Credo.Check.Refactor.RedundantWithClauseResult, []},

          #
          ## Warnings
          #
          {Credo.Check.Warning.ApplicationConfigInModuleAttribute, []},
          {Credo.Check.Warning.BoolOperationOnSameValues, []},
          {Credo.Check.Warning.ExpensiveEmptyEnumCheck, []},
          {Credo.Check.Warning.IExPry, []},
          {Credo.Check.Warning.IoInspect, []},
          {Credo.Check.Warning.OperationOnSameValues, []},
          {Credo.Check.Warning.OperationWithConstantResult, []},
          {Credo.Check.Warning.RaiseInsideRescue, []},
          {Credo.Check.Warning.SpecWithStruct, []},
          {Credo.Check.Warning.WrongTestFileExtension, []},
          {Credo.Check.Warning.UnusedEnumOperation, []},
          {Credo.Check.Warning.UnusedFileOperation, []},
          {Credo.Check.Warning.UnusedKeywordOperation, []},
          {Credo.Check.Warning.UnusedListOperation, []},
          {Credo.Check.Warning.UnusedPathOperation, []},
          {Credo.Check.Warning.UnusedRegexOperation, []},
          {Credo.Check.Warning.UnusedStringOperation, []},
          {Credo.Check.Warning.UnusedTupleOperation, []},
          {Credo.Check.Warning.UnsafeExec, []},

          # CredoNaming - tried both version below
          # {CredoNaming.Check.Consistency.ModuleFilename}
          {CredoNaming.Check.Consistency.ModuleFilename, []}
        ],
        disabled: [
          #
          # Checks scheduled for next check update (opt-in for now, just replace `false` with `[]`)

          #
          # Controversial and experimental checks (opt-in, just move the check to `:enabled`
          #   and be sure to use `mix credo --strict` to see low priority checks)
          #
          {Credo.Check.Consistency.MultiAliasImportRequireUse, []},
          {Credo.Check.Consistency.UnusedVariableNames, []},
          {Credo.Check.Design.DuplicatedCode, []},
          {Credo.Check.Design.SkipTestWithoutComment, []},
          {Credo.Check.Readability.AliasAs, []},
          {Credo.Check.Readability.BlockPipe, []},
          {Credo.Check.Readability.ImplTrue, []},
          {Credo.Check.Readability.MultiAlias, []},
          {Credo.Check.Readability.NestedFunctionCalls, []},
          {Credo.Check.Readability.SeparateAliasRequire, []},
          {Credo.Check.Readability.SingleFunctionToBlockPipe, []},
          {Credo.Check.Readability.SinglePipe, []},
          {Credo.Check.Readability.Specs, []},
          {Credo.Check.Readability.StrictModuleLayout, []},
          {Credo.Check.Readability.WithCustomTaggedTuple, []},
          {Credo.Check.Refactor.ABCSize, []},
          {Credo.Check.Refactor.AppendSingleItem, []},
          {Credo.Check.Refactor.DoubleBooleanNegation, []},
          {Credo.Check.Refactor.FilterReject, []},
          {Credo.Check.Refactor.IoPuts, []},
          {Credo.Check.Refactor.MapMap, []},
          {Credo.Check.Refactor.ModuleDependencies, []},
          {Credo.Check.Refactor.NegatedIsNil, []},
          {Credo.Check.Refactor.PipeChainStart, []},
          {Credo.Check.Refactor.RejectFilter, []},
          {Credo.Check.Refactor.VariableRebinding, []},
          {Credo.Check.Warning.LazyLogging, []},
          {Credo.Check.Warning.LeakyEnvironment, []},
          {Credo.Check.Warning.MapGetUnsafePass, []},
          {Credo.Check.Warning.MixEnv, []},
          {Credo.Check.Warning.UnsafeToAtom, []}

          # {Credo.Check.Refactor.MapInto, []},

          #
          # Custom checks can be created using `mix credo.gen.check`.
          #
        ]
      }
    }
  ]
}

Am I missing something?

I also tried to add into CredoNaming in the require and plugin sections, but it's not working.

Thanks.

Edit:

Forgot to mention that in order to test I only added the following module test_naming.exs in lib:

# test_naming.exs
defmodule FooBar do
  # ...
end
@remi remi added the bug Something isn't working label Feb 21, 2022
@remi
Copy link
Member

remi commented Feb 21, 2022

Hi @altdsoy,

First, thanks for the thorough report 🙂

I just ran the test suite after updating to credo 1.6 and it passed successfully:

$ mix deps | grep credo
* credo (Hex package) (mix)
  locked at 1.6.3 (credo) 1167cde0

$ mix test
Excluding tags: [to_be_implemented: true]

..............................

Finished in 0.1 seconds
30 tests, 0 failures

Randomized with seed 293833

So I don’t think a conflict with credo 1.6 and credo_naming is the issue here.

A few questions:

  • Are you able to generate errors from another check into your lib/test_naming.exs file? If not, that could be a sign that the file is ignored by your configuration.
  • What happens if you change strict: false to strict: true? The ModuleFilename check has a low priority, it might get skipped.

Thank you!

@altdsoy
Copy link
Author

altdsoy commented Feb 21, 2022

Hello and thanks for the reply.

Are you able to generate errors from another check into your lib/test_naming.exs file? If not, that could be a sign that the file is ignored by your configuration.

Yes. For example, when I add a simple IO.inspect, I'm getting a warning from credo.

What happens if you change strict: false to strict: true? The ModuleFilename check has a low priority, it might get skipped.

Ok this seems to do it. It worked!

Since it's within a Phoenix project I'm getting many (low [D]) level warning Nested modules could be aliased at the top of the invoking module. from generated Phoenix files (namely every test/support/*_case.ex files.

But this is weird because they all come from Credo.Check.Design.AliasUsage which are already within the enabled key as seen above...
So now I wonder what makes the strict flag enabling or not some checks already in the enabled sections?

I guess I can simply disable or change the options for that check so that I can use CredoNaming without changing any Phoenix files.

Though, CredoNaming triggers so many inconsistencies anyway for Phoenix controllers, view, etc... that I still need to handle them somehow.. Which if we should think strictly it's true, but those naming also makes sense the way they are..

So do you have a suggestion for some rules to add to ignore them?

More generally, how I can add a rule that allows me to have things like FooThing.ex to be inside things/foo_thing.ex. Which applies for example for plugs, tasks, controllers, views etc..
Maybe it could be also set as a default for CredoNaming?

For example, for something named FooThing it would have make more sense to have it under things/foo.ex (and by the way having also that as a valid rule for CredoNaming would have been nice I think). Since the _thing.ex is redundant with the parent folder despite the plural/singular form.

The problem in that case is if I have now FooOtherThing inside other_things/foo.ex it could be difficult to search for files and we are ending with many foo.ex files that we don't know what they are unless we look at the parent folders (which is the case with Phoenix Views, Controllers etc.).

Sure this can be solved within one's IDE, but still..

Anyway, I guess this issue is somehow solved. So feel free to close the issue.

Thanks for the plugin BTW.

Edit: I forgot to explicitly ask, how I can make CredoNaming work without the strict: true and if it was intended to be used this way or not?

@stevelounsbury
Copy link

Edit: I forgot to explicitly ask, how I can make CredoNaming work without the strict: true and if it was intended to be used this way or not?

I came across this problem and wanted to update the issue for others finding their way here. You can override the base_priority set by the credo check in your credo config by passing priority: :high (use one of :higher, :high, :normal, :low or :ignore):

 {CredoNaming.Check.Consistency.ModuleFilename, [priority: :high]},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants