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

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

Merged

Conversation

frerich
Copy link
Contributor

@frerich frerich commented Sep 30, 2023

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.

@frerich frerich force-pushed the detect_compositions_of_utc_now_and_truncate branch from 016fc54 to 26c6671 Compare September 30, 2023 17:32
….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.
I believe this refactoring is universally plausible, so let's enable it
by default.
@frerich frerich force-pushed the detect_compositions_of_utc_now_and_truncate branch from 26c6671 to bff1668 Compare October 1, 2023 18:48
Copy link
Owner

@rrrene rrrene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really good. Just some housekeeping items 👍

lib/credo/check/refactor/utc_now_truncate.ex Outdated Show resolved Hide resolved
lib/credo/check/refactor/utc_now_truncate.ex Outdated Show resolved Hide resolved
issue_meta,
message:
"Pass time unit to `{Naive}DateTime.utc_now` instead of composing with `{Naive}DateTime.truncate/2`.",
trigger: trigger,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

155b9d5 fixes this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@frerich frerich Dec 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lib/credo/check/refactor/utc_now_truncate.ex Outdated Show resolved Hide resolved
test/credo/check/refactor/utc_now_truncate_test.exs Outdated Show resolved Hide resolved
@frerich
Copy link
Contributor Author

frerich commented Dec 16, 2023

@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.

This should be a quote from the source code,
Credo.Check.add_column_if_missing/5 relies on this to calculate the
column value for the issue.
The `line_no` value should be the line where the `trigger` appears, i.e.
we should be using the meta information of the `truncate` AST node here.
@rrrene
Copy link
Owner

rrrene commented Dec 29, 2023

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 rrrene merged commit dd69577 into rrrene:master Feb 8, 2024
12 of 13 checks passed
@rrrene
Copy link
Owner

rrrene commented Feb 8, 2024

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! 🎉

@frerich frerich deleted the detect_compositions_of_utc_now_and_truncate branch February 12, 2024 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants