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

Fix layout false negatives around heredocs #9984

Merged
merged 3 commits into from Aug 7, 2021

Conversation

dvandersluis
Copy link
Member

@dvandersluis dvandersluis commented Aug 5, 2021

This fixes false negatives involving heredocs for Layout/SpaceBeforeComma, Layout/SpaceBeforeComment, Layout/SpaceBeforeSemicolon and Layout/SpaceInsideParens.

These cops rely on processed_source.tokens to determine layout, but heredocs mix up the order of tokens which causes layout issues after heredocs to be missed. This change uses processed_source.sorted_tokens to resolve this.

Because that method is currently private, in this PR each cop has a new private method that calls processed_source.send(:sorted_tokens). I have opened rubocop/rubocop-ast#195 to move that method to be public, after which I will replace all these methods.

Requires rubocop-ast >= 1.9.0.


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.

Copy link
Member

@koic koic left a comment

Choose a reason for hiding this comment

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

This looks good to me. On the other hand, I'm a little worried about the dependence on the private method of rubocop-ast. How about waiting for the release of rubocop/rubocop-ast#195 for a moment?

Cc @marcandre

@dvandersluis
Copy link
Member Author

dvandersluis commented Aug 6, 2021

Yep I agree, I used the private method as a temporary measure to get this PR up but I’d prefer to have my AST PR merged / released first too 😄

@marcandre
Copy link
Contributor

sorted_tokens is now public.

I checked out this PR. The cop checking for comments is an example that could be optimized (could start from comments instead of scanning all tokens). Other cops seem completely legit though (e.g. checking for ;).

If ever someone checks the performance and these cops are kind of slow, we could look into optimizing them, e.g. by adding some callbacks like on_semicolon_token or a general on_token and a constant for those that are interesting, but it's probably overkill.

@dvandersluis
Copy link
Member Author

@marcandre thanks for your feedback. Are you good with leaving performance optimizations for these cops to a different PR?

@marcandre
Copy link
Contributor

@marcandre thanks for your feedback. Are you good with leaving performance optimizations for these cops to a different PR?

Oh, absolutely. And unless these cops are in the top 20 of the slowest cops, there's not even a reason to optimize them, really.

@dvandersluis dvandersluis merged commit d572439 into rubocop:master Aug 7, 2021
@dvandersluis dvandersluis deleted the heredoc-spacing branch September 14, 2021 16:04
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

3 participants