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 false positive for Rails/LinkToBlank when rel is a symbol value #6869

Merged

Conversation

r7kamura
Copy link
Contributor

@r7kamura r7kamura commented Apr 1, 2019

This code should be valid on Rails/LinkToBlank cop, but it's not on rubocop v0.66.0 because rel value is a Symbol.

link_to 'Click here', 'https://www.example.com', target: :_blank, rel: :noopener

This PR is related to #6868.


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.

@r7kamura r7kamura force-pushed the feature/rails-link-to-blank-symbol-rel branch from 1a997e7 to 77282a0 Compare April 1, 2019 01:08
CHANGELOG.md Outdated
@@ -8,6 +8,7 @@
* [#6856](https://github.com/rubocop-hq/rubocop/pull/6856): Fix auto-correction for `Style/BlockComments` when the file is missing a trailing blank line. ([@ericsullivan][])
* [#6858](https://github.com/rubocop-hq/rubocop/issues/6858): Fix an incorrect auto-correct for `Lint/ToJSON` when there are no `to_json` arguments. ([@koic][])
* [#6865](https://github.com/rubocop-hq/rubocop/pull/6865): Fix deactivated `StyleGuideBaseURL` for `Layout/ClassStructure`. ([@aeroastro][])
* [#6869](https://github.com/rubocop-hq/rubocop/pull/6869): Fix false positive for `Rails/LinkToBlank`. ([@r7kamura][])
Copy link
Member

Choose a reason for hiding this comment

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

Can you write a little more about the reason for false positive? It will be user friendly 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Slightly modified like this:

- Fix false positive for `Rails/LinkToBlank`.
+ Fix false positive for `Rails/LinkToBlank` when rel is a symbol value.

@r7kamura r7kamura force-pushed the feature/rails-link-to-blank-symbol-rel branch from 77282a0 to a8aebed Compare April 1, 2019 08:24
@r7kamura r7kamura changed the title Fix false positive for Rails/LinkToBlank on symbol rel Fix false positive for Rails/LinkToBlank when rel is a symbol value Apr 1, 2019
@r7kamura r7kamura force-pushed the feature/rails-link-to-blank-symbol-rel branch from a8aebed to 3138f0a Compare April 1, 2019 08:25
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! Can you resolve branch conflicts?

@@ -22,7 +22,7 @@ class LinkToBlank < Cop
PATTERN

def_node_matcher :includes_noopener?, <<-PATTERN
(pair {(sym :rel) (str "rel")} (str #contains_noopener?))
(pair {(sym :rel) (str "rel")} {(str #contains_noopener?) (sym #contains_noopener?)})
Copy link
Collaborator

@Drenmi Drenmi Apr 1, 2019

Choose a reason for hiding this comment

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

I could be wrong, but I think you could do:

({str sym} #contains_noopener?)

Don't take my word for it though. I haven't tested the pattern. 🙂

@@ -79,10 +79,10 @@ def add_rel(send_node, offence_node, corrector)
corrector.insert_after(range, new_rel_exp)
end

def contains_noopener?(str)
return false unless str
def contains_noopener?(value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@r7kamura r7kamura force-pushed the feature/rails-link-to-blank-symbol-rel branch from 3138f0a to 2350e8c Compare April 1, 2019 21:39
@r7kamura r7kamura force-pushed the feature/rails-link-to-blank-symbol-rel branch from 2350e8c to 8f6b191 Compare April 1, 2019 22:37
@bbatsov bbatsov merged commit 7d66f85 into rubocop:master Apr 2, 2019
@r7kamura r7kamura deleted the feature/rails-link-to-blank-symbol-rel branch April 2, 2019 13:52
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