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

Add Rails/DateAndTimeCalculation cop #853

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

r7kamura
Copy link
Contributor

@r7kamura r7kamura commented Nov 2, 2022

ActiveSupport provides a lot of useful but not well known date and time helpers. I think it would be nice if RuboCop could wisely recommend the use of these helpers.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.

@r7kamura r7kamura force-pushed the feature/date-and-time-calculation branch from ed1abc5 to fb54814 Compare November 2, 2022 02:18
@r7kamura r7kamura force-pushed the feature/date-and-time-calculation branch from fb54814 to 12453a6 Compare November 2, 2022 02:22
@r7kamura
Copy link
Contributor Author

r7kamura commented Nov 2, 2022

Slightly off topic, but I think the following two Cops are inconsistent in their names. They both deal with timezone issues for a particular class, yet one refers to the class and the other to the timezone.

  • Rails/Date
  • Rails/TimeZone

We migjht want to name as the following:

  • Rails/DateTimeZone
  • Rails/TimeTimeZone

or merge them into a single Cop:

  • Rails/TimeZone

I understand that it is a bit too late to talk about this now. I thought of that when I was considering whether to separate the Rails/DateAndTimeCalculation Cop into the following two Cops:

  • Rails/DateCalculation
  • Rails/TimeCalculation

The current name "DateAndTimeCalculation" is after DateAndTime::Calculations module in Rails.

# @!method comparison_to_time_current?(node)
# @param node [RuboCop::AST::Node]
# @return [Boolean]
def_node_matcher :comparison_to_time_current?, <<~PATTERN
Copy link
Member

Choose a reason for hiding this comment

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

Please move the def_node_matcher DSLs under constants. These are not published APIs and are not actually called externally. Mixing DSL and private methods is hard for maintainer to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll make that change 👌

I think it is a common belief that methods should be kept private unless necessary, and that in cases where callbacks are realized in template method pattern like this, that callbacks should be defined in a more prominent position, but in this case you say we should do so even if it breaks these expectations, correct?

It is a bit strange that this point is being discussed at the time of the addition of this Cop, since some of the cops should have already implemented it this way. If there is such an implicit rule, it would be nice to have an internal affair cop or at least have some documentation for that.

Copy link
Member

Choose a reason for hiding this comment

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

Reviewers don't always notice it, and they don't always point it out. Whether or not reviewers will comment will depend on the degree. Also, I don't want to retroactively change existing code because it is already in a sufficiently maintainable state.

@koic
Copy link
Member

koic commented Nov 2, 2022

Sure, the cop name and role will need time to be considered. Let's think without rushing because there is triage.

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