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 Style/RequireOrder cop #11235

Merged
merged 1 commit into from Dec 7, 2022
Merged

Conversation

r7kamura
Copy link
Contributor

@r7kamura r7kamura commented Dec 3, 2022

Many projects have require and require_relative in alphabetical order by convention, the rubocop gem itself being one example. I thought it would be good to have a cop to provide such consistency.

As a good example, rubocop-rspec inspects this with tests. Such checks are replaced by this cop.

Like Bundler/OrderedGems, code separated by a blank line is considered a different group, so if there are obvious dependencies but you really don't want to write require_relative in each file, it is better to separate the groups. lib/rubocop/formatter.rb is a good example of that.

Unfortunately, the dependencies written in lib/rubocop.rb are too complex, so I gave up on organizing this at the time of introducing this cop and decided to ignore it from this cop. Personally, if one file A depends on another file B, I prefer to indicate that fact by writing require_relative in file A, but I don't think that is the practice in the rubocop gem, so I don't want to go into this issue here.

Since this cop enforces the rule "always sort them in alphabetical order within a group", I don't think false-positive will occur, but applying autocorrection obviously changes the behavior from the original code, so I have set SafeAutoCorrect: false.

Ideally, I believe it is a good manner that the code to always be written in such a way that it does not depend on the order of require, but I set Enabled: false because this cop has a large impact and may or may not be acceptable depending on the preference of the project. I think this cop is rather opinionated, so I would be glad to get your opinion on the policy.


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.

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 5, 2022

I love this suggestion and the thoughtful analysis you've submitted alongside it! I'm only wondering if this cop should also suggest to keep require and require_relative sections ordered as well (e.g. require first, require_relative afterwards or vice-versa).

# require_relative 'a'
# require_relative 'b'
#
# # good
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might mention here that a blank line introduces a new section or something like this.

@r7kamura
Copy link
Contributor Author

r7kamura commented Dec 6, 2022

'm only wondering if this cop should also suggest to keep require and require_relative sections ordered as well (e.g. require first, require_relative afterwards or vice-versa).

I guess it would be difficult to enforce that, since there would be case where you want to write relative_require before require, or vice versa, depending on the situation. If we want to do that, it might be better to prepare something like Style/RequireSectionOrder separately.

@bbatsov bbatsov merged commit 77229bc into rubocop:master Dec 7, 2022
@zhuravel
Copy link
Contributor

zhuravel commented Dec 8, 2022

@r7kamura @bbatsov This cop fails for code like:

def load_account
  @account = Account.find(params.require(:account_id))
end

Trace:

An error occurred while Style/RequireOrder cop was inspecting /Users/bohdan/Projects/talkable/app/controllers/account/accesses_controller.rb:69:28.
undefined method `send_type?' for :account_id:Symbol

            break unless sibling.send_type?
                                ^^^^^^^^^^^
/Users/bohdan/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/rubocop-1.40.0/lib/rubocop/cop/style/require_order.rb:66:in `block in find_previous_older_sibling'

koic added a commit to koic/rubocop that referenced this pull request Dec 8, 2022
Follow up rubocop#11235 (comment).

This PR fixes an error for `Style/RequireOrder` when `require` is a method argument.
@koic koic mentioned this pull request Dec 8, 2022
8 tasks
@koic
Copy link
Member

koic commented Dec 8, 2022

@zhuravel Thank you for the feedback. That issue will be resolved by #11254.

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.

None yet

4 participants