Detect compositions of {Naive}DateTime.utc_now and {Naive}DateTime.truncate

[frerich]

This PR adds a new refactoring check which encourages people to pass a time unit to DateTime.utc_now.

Instead of calling DateTime.utc_now and then truncating the result with DateTime.truncate/2, it's more efficient (and less code) to just pass a time unit indicating the desired precision to DateTime.utc_now directly.

Dito for NaiveDateTime.

[rrrene]

Looking really good. Just some housekeeping items 👍

[rrrene]

The trigger has to be the string that causes the issue.

[frerich]

Ah, I see - will fix; I suppose the bit which is triggering the warning is the truncate/2 call, so I'll use that.

[frerich]

155b9d5 fixes this.

[rrrene]

I probably was unclear (sorry, not a native speaker).

:trigger has to be the actual piece of code causing the issue, which e.g. the editor should highlight (in this case probably {Naive}DateTime.truncate).

[frerich]

Sorry, maybe I'm being a little dense here: the trigger is a plain string, right? I looked at other checks and gathered that it should be a name of a function causing the check to trigger.

In the check at hand, the issue_for/3 function is invoked like

    new_issue = issue_for(issue_meta, meta[:line], "NaiveDateTime")

..and its definition is

  defp issue_for(issue_meta, line_no, module) do
    format_issue(
      issue_meta,
      message:
        "Pass time unit to `#{module}.utc_now` instead of composing with `#{module}.truncate/2`.",
      trigger: "#{module}.truncate/2",
      line_no: line_no
    )
  end

I.e. the third argument is a module name, and the trigger then becomes e.g. DateTime.truncate/2 or NaiveDateTime.truncate/2, depending on what the caller passed. Is that not doing the right thing? 🤔

[rrrene]

Yes, which is why I wrote

:trigger has to be the actual piece of code causing the issue, which e.g. the editor should highlight

and there will be no NaiveDateTime.truncate/2 in the code in your example.

There will e.g. be |> NaiveDateTime.truncate(:second) and here we would want the NaiveDateTime.truncate part to be identified as the piece of code that "triggered" the issue (thus being the :trigger).

[frerich]

Ah, I see - it needs to be a literal quote from the code, so the trigger should be just #{module}.truncate then!

I suppose this also means that the line_no value should not be the line of the |> node but rather of the truncate expression itself. I see that Credo.Check.add_column_if_missing/5 relies on this to calculate the column value for the issue. Will add another commit to do this which can be squashed later.

I wonder what this all means for aliases, e.g.

alias DateTime, as: DT

DT.utc_now()
|>
DT.truncate(:second)

I suppose in this case, DT.truncate is the trigger to be mentioned in the issue. Will see if this can be made to work.

[frerich]

Adjusted the check now such that it reports a correct trigger (cf. aa498e1) and that the line number reported is always the line in which the trigger appears (cf. 301397d). Looks better now? 🤗

I briefly looked into supporting aliased uses of DateTime, but I believe this would amount to a significant rewrite of the check which is maybe worth a separate PR in the future. I think the logic might be roughly:

• First, traverse the AST and stop at the defmodule node.
• Once the defmodule node is visited, call Credo.Code.Module.aliases/1 to get all aliases in the module and then start a recursive prewalk of the AST, passing the aliases along to the callback.
• In the recursive walk of the AST, don't pattern-match on the DateTime module directly. Instead, have each pattern extract the referenced module name and then test if the name is among the known aliases (or DateTime itself). Alas, since the list of aliases is not a compile-time list, this can not be done using a guard.

[frerich]

@rrrene thanks for the diligent review, I addressed your comments and pushed some additional commits (instead of squashing) to simplify review. If this looks okay, I guess it would be nice to squash the four most recent commits into 1a28a23.

[rrrene]

Perfect. I think we can merge this as is 🎉

I will have to make a new branch, as we only introduce new checks in minor changes. So this will become part of Credo 1.8 ... I would target a release window of end of Jan, beginning of Feb 👍

[rrrene]

Hi again,

this is now part of Credo 1.7.4. The next minor release will have to wait a bit, but I wanted to get this check in the hands of people before that.

It is now "opt-in", meaning people have to activate it. But we will include this as a standard check that is enabled by default in the future. 👍

Sorry for the inconvenience, I should have had the idea earlier to include this as an "opt in" check for now and make it enabled by default later.

Thx again for your contribution! 🎉