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

Make Rails/ExpandedDateRange aware beginning_of_week with an argument #730

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

koic
Copy link
Member

@koic koic commented Jun 25, 2022

This PR makes Rails/ExpandedDateRange aware beginning_of_week with an argument.

# bad
date.beginning_of_week(:sunday)..date.end_of_week(:sunday)

# good
date.all_week(:sunday)

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.

koic added a commit to koic/rails-style-guide that referenced this pull request Jun 25, 2022
…e/time" rule

Follow up rubocop/rubocop-rails#730.

This PR adds new "Prefer `all_(day|week|month|quarter|year)` over range of date/time" rule.

```ruby
# bad
date.beginning_of_day..date.end_of_day
date.beginning_of_week..date.end_of_week
date.beginning_of_month..date.end_of_month
date.beginning_of_quarter..date.end_of_quarter
date.beginning_of_year..date.end_of_year

# good
date.all_day
date.all_week
date.all_month
date.all_quarter
date.all_year
```
@koic koic force-pushed the add_new_rails_date_time_range_cop branch 4 times, most recently from 6f4a44d to 3994741 Compare June 26, 2022 15:33
@pirj
Copy link
Member

pirj commented Jun 27, 2022

An error occurred while Rails/DateTimeRange cop was inspecting /Users/pirj/source/real-world-rspec/gitlabhq/app/models/broadcast_message.rb:122:5.
undefined method `source' for nil:NilClass

            range_begin.receiver.source != range_end.receiver.source ||
                                ^^^^^^^
/Users/pirj/source/rubocop-rails/lib/rubocop/cop/rails/date_time_range.rb:69:in `allow?'
/Users/pirj/source/rubocop-rails/lib/rubocop/cop/rails/date_time_range.rb:50:in `on_irange'

I guess it is this line.

I see a number of similar errors when running it against real-world-rspec repos.

@koic koic force-pushed the add_new_rails_date_time_range_cop branch from 3994741 to f0bb668 Compare June 29, 2022 10:02
@koic
Copy link
Member Author

koic commented Jun 29, 2022

I've updated this PR. Thank you @pirj!

@pirj
Copy link
Member

pirj commented Jun 29, 2022

Nice!

Running on the same corpus:

57497 files inspected, 6 offenses detected, 6 offenses autocorrectable

/Users/pirj/source/real-world-rspec/discourse/app/jobs/concerns/skippable.rb:22:22: C: [Correctable] Rails/DateTimeRange: Use Time.zone.now.all_day instead.
        created_at: (Time.zone.now.beginning_of_day..Time.zone.now.end_of_day)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/discourse/lib/guardian.rb:463:54: C: [Correctable] Rails/DateTimeRange: Use Time.zone.now.all_day instead.
    UserExport.where(user_id: @user.id, created_at: (Time.zone.now.beginning_of_day..Time.zone.now.end_of_day)).count == 0
                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/gitlabhq/app/models/member.rb:157:23: C: [Correctable] Rails/DateTimeRange: Use now.all_day instead.
    where(created_at: now.beginning_of_day..now.end_of_day)
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/gitlabhq/app/services/namespaces/in_product_marketing_emails_service.rb:144:7: C: [Correctable] Rails/DateTimeRange: Use date.all_day instead.
      date.beginning_of_day..date.end_of_day
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/gitlabhq/lib/gitlab/contributions_calendar.rb:62:28: C: [Correctable] Rails/DateTimeRange: Use date_in_time_zone.all_day instead.
        .where(created_at: date_in_time_zone.beginning_of_day..date_in_time_zone.end_of_day)
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
/Users/pirj/source/real-world-rspec/gitlabhq/spec/models/onboarding_progress_spec.rb:72:81: C: [Correctable] Rails/DateTimeRange: Use 1.day.ago.all_day instead.
      subject { described_class.completed_actions_with_latest_in_range(actions, 1.day.ago.beginning_of_day..1.day.ago.end_of_day) }
                                                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

replacement = replacement(range_begin)

if range_begin.method?(:beginning_of_week) && range_begin.arguments.one?
return unless same_argument?(range_begin, range_end)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call it same_receiver??

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an intended naming because it confirms the argument of beginning_of_week.

end

def replacement(range_begin)
+"#{range_begin.receiver.source}.#{PREFER_METHODS[range_begin.method_name]}"
Copy link
Member

Choose a reason for hiding this comment

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

Does it need a +? It won't be frozen, since it contains interpolation, will it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it's a feature from Ruby 3.0.

% cat example.rb
# frozen_string_literal: true

value = 'value'

p "#{value}".frozen?

Ruby 2.7

% ruby -ve xample.rb
ruby 2.7.6p219 (2022-04-12 revision c9c2245c0a) [x86_64-darwin19]
true

Ruby 3.0

% ruby -v example.rb
ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-darwin19]
false

Copy link
Member

Choose a reason for hiding this comment

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

TIL, thank you!

@pirj
Copy link
Member

pirj commented Jun 29, 2022

Checked manually for some potential offences:

Invoice.where(invoice_date: Date.today.beginning_of_year..Date.today)

This may be unsafe to correct, as we don't really know if there are dates in the future. All good.

@pirj
Copy link
Member

pirj commented Jun 29, 2022

⚠️ Funny enough, the search found a cop that already exists and looks identical https://github.com/rubocop/rubocop-rails/blob/master/lib/rubocop/cop/rails/expanded_date_range.rb

@koic
Copy link
Member Author

koic commented Jun 30, 2022

Oops! I completely overlooked it... On the other hand, this implementation is better to detecting the beginning_of_week with argument. I will update this PR as an extension to Rails/ExpandedDateRange cop instead of adding this cop.

@koic koic force-pushed the add_new_rails_date_time_range_cop branch from f0bb668 to 8889e1c Compare June 30, 2022 06:53
…ment

This PR makes `Rails/ExpandedDateRange` aware `beginning_of_week` with an argument.

```ruby
# bad
date.beginning_of_week(:sunday)..date.end_of_week(:sunday)

# good
date.all_week(:sunday)
```
@koic koic force-pushed the add_new_rails_date_time_range_cop branch from 8889e1c to 15858ae Compare June 30, 2022 06:55
@koic koic changed the title Add new Rails/DateTimeRange cop Make Rails/ExpandedDateRange aware beginning_of_week with an argument Jun 30, 2022
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!
I'm running low on disk space, can't check on real-world-rails now.
And I guess the code is the same as for the previous run, so it makes no sense to run it on real-world-rspec again.

@koic koic merged commit a20671e into rubocop:master Jul 7, 2022
@koic koic deleted the add_new_rails_date_time_range_cop branch July 7, 2022 01:45
goldapple911 added a commit to goldapple911/rails-style-guide that referenced this pull request Aug 15, 2023
…e/time" rule

Follow up rubocop/rubocop-rails#730.

This PR adds new "Prefer `all_(day|week|month|quarter|year)` over range of date/time" rule.

```ruby
# bad
date.beginning_of_day..date.end_of_day
date.beginning_of_week..date.end_of_week
date.beginning_of_month..date.end_of_month
date.beginning_of_quarter..date.end_of_quarter
date.beginning_of_year..date.end_of_year

# good
date.all_day
date.all_week
date.all_month
date.all_quarter
date.all_year
```
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