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 #7728] Fix an error for Style/OneLineConditional #7729

Merged

Conversation

koic
Copy link
Member

@koic koic commented Feb 19, 2020

Fixes #7728.

This PR fixes an error for Style/OneLineConditional when one of the branches contains a self keyword.


Before submitting the PR make sure the following are checked:

  • 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.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

lib/rubocop/ast/node/self_node.rb Outdated Show resolved Hide resolved
@koic koic force-pushed the fix_an_error_for_style_one_line_conditional branch from e6393fe to 35cb74f Compare February 19, 2020 05:58
include MethodIdentifierPredicates

# Always return `false` because `self` cannot have arguments.
def arguments?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you point me to the code that calls #arguments? on a self node? I think the call site probably should filter the relevant nodes instead. I can have a look. 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

SelfNode#arguments? is called in the following code.
https://github.com/rubocop-hq/rubocop/blob/v0.80.0/lib/rubocop/cop/style/one_line_conditional.rb#L96

I was worried about choosing between implementing SelfNode#arguments? or guarding the above with resopnd_to(:arguments?).

  def keyword_with_changed_precedence?(node)
    return false unless node.keyword?
    return true if node.prefix_not?

-   node.arguments? && !node.parenthesized_call?
+   node.respond_to?(:arguments?) && node.arguments? && !node.parenthesized_call?
  end

Copy link
Collaborator

Choose a reason for hiding this comment

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

@koic Let me have a look. 🙂 Ideally we shouldn't do any of those. We should make sure incompatible nodes don't reach this code.

@koic koic force-pushed the fix_an_error_for_style_one_line_conditional branch from 35cb74f to f7b8edc Compare February 23, 2020 15:42
@koic koic force-pushed the fix_an_error_for_style_one_line_conditional branch 2 times, most recently from cd14a0c to 30dfc79 Compare March 8, 2020 11:23
@Drenmi Drenmi self-assigned this Mar 9, 2020
@koic koic force-pushed the fix_an_error_for_style_one_line_conditional branch from 30dfc79 to b4b5ae2 Compare March 21, 2020 16:08
@tas50
Copy link
Contributor

tas50 commented Mar 24, 2020

@koic Any chance this one can get merged so I can do testing on my side?

Fixes rubocop#7728.

This PR fixes an error for `Style/OneLineConditional`
when one of the branches contains a self keyword.
@koic koic force-pushed the fix_an_error_for_style_one_line_conditional branch from b4b5ae2 to 8815dda Compare March 25, 2020 02:13
@koic
Copy link
Member Author

koic commented Mar 26, 2020

@Drenmi Can we merge this PR to resolve the error first?

@Drenmi
Copy link
Collaborator

Drenmi commented Mar 27, 2020

@koic Yes! Please go ahead. I don't have time to look into it for another couple of weeks or so. 🙂

@koic
Copy link
Member Author

koic commented Mar 27, 2020

I get it. Thank you for taking your time!

@koic koic merged commit 5d47450 into rubocop:master Mar 27, 2020
@koic koic deleted the fix_an_error_for_style_one_line_conditional branch March 27, 2020 05:01
koic added a commit to koic/rubocop that referenced this pull request Mar 30, 2020
Fixes rubocop#7829.

This PR fixes an error for `Style/OneLineConditional`
when one of the branches contains `next` keyword.

`SelfNode` was introduced by rubocop#7729 to prevent `NoMethodErrror`,
but it is unnecessary because the cop that caused the error were
fixed.
bbatsov pushed a commit that referenced this pull request Mar 30, 2020
Fixes #7829.

This PR fixes an error for `Style/OneLineConditional`
when one of the branches contains `next` keyword.

`SelfNode` was introduced by #7729 to prevent `NoMethodErrror`,
but it is unnecessary because the cop that caused the error were
fixed.
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.

undefined method `prefix_not? in Style/OneLineConditional
4 participants