MissedMetadataKeyInLoggerConfig shouldn't warn if you don't use the console logger backend

[mhanberg]

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.7.0-rc.1
• Erlang/Elixir version (elixir -v):

Erlang/OTP 25 [erts-13.1.3] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1] [jit]

Elixir 1.14.3 (compiled with Erlang/OTP 25)

• Operating system: macOS 13

What were you trying to do?

I was going to test out the RC release, and noticed we get a lot of warning for the new check MissedMetadataKeyInLoggerConfig.

We do not use the :console logger backend in production, so these are false positives.

Expected outcome

Should only warn if the :console backend is configured for that env.

Actual outcome

False positives.

cc @milmazz as the author of this new check in #952

[mhanberg]

I can submit a patch to change the behavior if this seems appropriate.

[rrrene]

Understood. The only thing that is important is that we can not compile or execute code from the analyzed project in Credo, because it is a static analysis tool that "just looks at the code". We might already have violated that rule with the check's current implementation, which does not work for situations where --working-dir is set to some other (not loaded) Mix project.

Maybe we should just provide an example to configure the check (as proposed by @milmazz at the start of his original PR):

# uncomment an modify this to your needs:
{Credo.Check.Warning.IgnoredLoggerMetadata, [metadata_keys: "config/prod.exs" |> Config.Reader.read!() |> get_in([:logger, :console, :metadata])]},

we can do this because .credo.exs is just a regular Elixir script and if the user leverages this fact to read the project´s actual config and provides that as a parameter to the check, then we're fine.

WDYT?

[mhanberg]

I upgraded to 1.7 today and I ended up just disabling this check.

[angelikatyborska]

Maybe we should just provide an example to configure the check

That would be a good improvement. In the check's explanation? I had to configure the check in my project to make it work too, for the same reason as is the topic of this issue 🙂

In case anyone is reading this thread half-asleep, rrrene's example needs to be modified to work:

{Credo.Check.Warning.IgnoredLoggerMetadata, [metadata_keys: "config/prod.exs" |> Config.Reader.read!() |> get_in([:logger, :console, :metadata])]},

First, the check's module name is different. Second, if you hit this problem because you don't have the console backend in production, you need to pass the name of your actual logger backend. In my case it's :file_log. This config worked for me:

  {Credo.Check.Warning.MissedMetadataKeyInLoggerConfig,
   [
     metadata_keys:
       "config/prod.exs" |> Config.Reader.read!() |> get_in([:logger, :file_log, :metadata])
   ]}

[rrrene]

This is part of 1.7.2-rc.3 👍

[rrrene]

This is part of Credo 1.7.2. 👍