Question around .credo.exs merge

epinault commented 1 year ago

Precheck

Proposals for new features should be submitted via: https://github.com/rrrene/credo-proposals
For bugs, please do a quick search and make sure the bug has not yet been reported here

Environment

Credo version (mix credo -v): 1.6.7-ref.master.3ee5708+uncommittedchanges
Erlang/Elixir version (elixir -v):
- Erlang/OTP 25 [erts-13.2.2.1] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1] [jit]
- Elixir 1.14.5 (compiled with Erlang/OTP 24)
Operating system: OSX Ventura

What were you trying to do?

I am trying to build a plugin. I have a simple plugin as a library that loads a .credo.exs file within it its own repo. That part works fine

But when mixing with using that package in another repo (Say MyCoolApp), I am noticing a strange behavior around mergine the enabled checks

If my .credo.exs in MyCoolApp repo contains

  #
  # You can have as many configs as you like in the `configs:` field.
  configs: [
    %{
      name: "default",
      files: %{
        included: [
          "lib/",
          "src/",
          "test/",
          "web/",
          "apps/*/lib/",
          "apps/*/src/",
          "apps/*/test/",
          "apps/*/web/"
        ],
        excluded: [~r"/_build/", ~r"/deps/", ~r"/node_modules/"]
      },
      plugins: [        {MyCredoPlugin, []}],
      requires: [],
      strict: true,
      parse_timeout: 5000,
      color: true,
      checks: %{
        enabled: []}
}

it seems that all the checks coming from my plugin are now disabled .

If I remove the

      checks: %{
        enabled: []}

then it seems that it works as intended and I get the default checks from my lib. what am I missing or doing wrong? Does the name need to be different in order to get a proper merge? or that makes no difference?

Expected outcome

Outcome is that I am merging the checks from my lib into my local file as described in https://hexdocs.pm/credo/config_file.html#transitive-configuration-files

but then again I am not sure how the merge happens when you enable a check in one and disable it in a upper folder or after your defaults

Actual outcome

currently seems that it disables all of my checks but I want to be able to pick and choose/ override in my MyCoolApp check from the plugin that configured

rrrene commented 1 year ago

what am I missing or doing wrong?

The config setting

 checks: %{
         enabled: []}

says "enable no checks", so I am not sure why you are surprised that checks are disabled?

I realise that the documentation is lacking in this regard. Maybe this changelog entry might help? https://hexdocs.pm/credo/changelog.html#pinning-checks-in-a-project-s-config

epinault commented 1 year ago

I see. So if I have my own default file, and I just want to disable specific one that is possible ?

like say I have a global file that comes as part of the plugin I built, and the want a local app to turn off a rule, can I use the disable instead and it won t override all the enable one?

My goal is to centralize rules in a package so apply to most of our application but leave some rroom temporarily for older apps to not comply yet with some rules that are a lot of work to fix

rrrene commented 1 year ago

can I use the disable instead and it won t override all the enable one?

Exactly :+1:

epinault commented 1 year ago

ok thanks!

epinault commented 1 year ago

actually, I am reopening because I am seeing something (that works for me but is not clear)

if I have a default file in my plugin as before, and I want to override the rule It seems that I can do it this way in the current application. and that overridle the check set my default plugin

checks: [
        {Credo.Check.Refactor.ABCSize, [max_size: 35]},
        {Credo.Check.Refactor.ModuleDependencies, [max_deps: 15]}
      ]

if I try to override with the enabled config , it does not work and override the entire enabled section so I loose other checks I needed

checks: %{
       enabled: [
        {Credo.Check.Refactor.ABCSize, [max_size: 35]},
        {Credo.Check.Refactor.ModuleDependencies, [max_deps: 15]}
       ]
      }

But now I have the case of:

I need to disable some checks but also override the config for some checks. I cannot seem to find a way,

I tried to go with subfolder .credo.exs file and that did not make a difference (does not even seem to pick up that file. so say I ahve top file .credo.exs and a lib/somefolder/.credo.exs with this

      checks: [
        {Credo.Check.Readability.Specs,
         files: %{
           excluded: [
             "**/*.ex"
           ]
         }}
      ]

that check configuration seems to not work merging with the top application and the one from the plugin

rrrene commented 1 year ago

it does not work and override the entire enabled section so I loose other checks I needed

That is correct. What you want to do is use the :extra key as described here: https://hexdocs.pm/credo/1.7.0/config_file.html#checks

so say I ahve top file .credo.exs and a lib/somefolder/.credo.exs with this

That is not how .credo.exs files work. You cannot put them into lib/. Here is more info: https://hexdocs.pm/credo/1.7.0/config_file.html#transitive-configuration-files

epinault commented 1 year ago

ok thanks! I think I am more clear about the transitive now. I thought somehow I could use it under lib

rrrene commented 1 year ago

@epinault no worries. And for what it is worth: I updated the docs thanks to this issue, so everyone wins. ✌️

epinault commented 1 year ago

awesome ! thank you!

rrrene / credo