-
-
Notifications
You must be signed in to change notification settings - Fork 263
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 #1288] Let LinkToBlank find violations in link_to_if and link_to_unless #1289
[Fix #1288] Let LinkToBlank find violations in link_to_if and link_to_unless #1289
Conversation
@@ -1,6 +1,18 @@ | |||
# frozen_string_literal: true | |||
|
|||
RSpec.describe RuboCop::Cop::Rails::LinkToBlank, :config do | |||
['link_to', 'link_to_if condition?, ', 'link_to_unless condition?, '].each do |link_helper_method| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Examplary. All other specs in here could be wrapped like this (70db8e4)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you divide this into three separate tests instead of using each
loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it will be three basically identically long blocks of tests then, but I understand if you prefer the explicitness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, redundant commas like in with 'link_to_unless condition?, '
will be included in the text of context
. To avoid this, it is acceptable to write three similar tests that only differ in the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, did that in the last commit for all the specs. If acceptable, I will squash the commits/drop the old one.
fe22f72
to
a3138ef
Compare
@fwolfst Looks good to me. Can you squash your commits into one? |
of the LinkToBlank cop. Add changelog entry and add specs within three contexts for the three different methods Co-authored-by: Koichi ITO <[email protected]>
a3138ef
to
e8d60f3
Compare
done. |
Thanks! |
Fix #1288 (but not adding link_to_unless_current) by adding respective methods to the
RESTRICT_ON_SEND
list, hoping that this is the right approach.I sketched an approach for the specs, but it would mean executing very similar specs three times - see relevant commit for a very brief discussion: 70db8e4 . What do you think?
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.