Implement autocorrect for Capybara/CurrentPathExpectation #641

ypresto · 2018-06-13T13:27:54Z

Capybara/CurrentPathExpectation did not have autocorrect so I implemented it!

Note that match(variable) is not supported due to ambiguous replacement:

If variable returns string, it should be replaced with have_current_path(/#{variable}/).
If variable returns regexp, it should be replaced with have_current_path(variable).

Before submitting the PR make sure the following are checked:

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.
The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

ypresto · 2018-06-13T14:11:16Z

Possible bug: if current_path is overridden in current context (let, local variable or helper method), it can cause false-positive.

bquorning

Overall, this looks really good. I am a bit concerned about the two matchers. The first is quite complicated, and the 2nd somewhat repeats part of the 1st one.

What are the downsides of splitting the 1st matcher in two, one being specific to the :match(str $_) pattern? That would reduce duplication in the matchers, but perhaps introduce extra duplication further down in the class…

bquorning · 2018-06-15T20:38:55Z

lib/rubocop/cop/rspec/capybara/current_path_expectation.rb

+
+          def autocorrect(node)
+            lambda do |corrector|
+              return unless node.chained?


There is no test case covering this line.

added test!

Add empty line after the return

bquorning · 2018-06-15T20:49:16Z

lib/rubocop/cop/rspec/capybara/current_path_expectation.rb

+            current_path_node = node.first_argument
+            corrector.replace(current_path_node.loc.expression, 'page')
+            corrector.replace(node.parent.loc.selector, 'to')
+            matche_method = if to_symbol == :to


I think this is a typo: matche_method -> matcher_method.

ypresto · 2018-06-25T07:44:07Z

What are the downsides of splitting the 1st matcher in two

Firstly I'm putting all the code into body of def autocorrect so much of duplication with two (large) matchers pattern. But it can be simpler now:

def autocorrect(node)
    ...
    as_is_matcher(node.parent) do |to_symbol, matcher_node|
      rewrite_expectation(corrector, node, to_symbol, matcher_node)
    end
    regexp_str_matcher(node.parent) do |to_symbol, matcher_node|
      rewrite_expectation(corrector, node, to_symbol, matcher_node)
      convert_regexp_str_to_literal(corrector, matcher_node)
    end
    ...
end

ypresto · 2018-08-02T02:04:56Z

Thank you for reviewing, finally fixed them! :)

bquorning

You need to rebase on master, but other than that I think this PR is fine.

@Darhazer, any comments?

Darhazer · 2018-08-13T16:50:25Z

lib/rubocop/cop/rspec/capybara/current_path_expectation.rb

+          # Supported matchers: eq(...) / match(/regexp/) / match('regexp')
+          def_node_matcher :as_is_matcher, <<-PATTERN
+            (send
+              (send nil? :expect (send {(send nil? :page) nil?} :current_path)) ${:to :not_to :to_not}


At least reuse the expectation_set_on_current_path - you can write

#expectation_set_on_current_path ${:to :not_to :to_not}

Darhazer · 2018-08-13T16:50:34Z

lib/rubocop/cop/rspec/capybara/current_path_expectation.rb

+
+          def_node_matcher :regexp_str_matcher, <<-PATTERN
+            (send
+              (send nil? :expect (send {(send nil? :page) nil?} :current_path)) ${:to :not_to :to_not}


reuse expectation_set_on_current_path

Darhazer · 2018-08-13T16:50:45Z

lib/rubocop/cop/rspec/capybara/current_path_expectation.rb

+
+          def autocorrect(node)
+            lambda do |corrector|
+              return unless node.chained?


Add empty line after the return

Darhazer · 2018-08-13T16:52:36Z

lib/rubocop/cop/rspec/capybara/current_path_expectation.rb

+              as_is_matcher(node.parent) do |to_sym, matcher_node|
+                rewrite_expectation(corrector, node, to_sym, matcher_node)
+              end
+              regexp_str_matcher(node.parent) do |to_sym, matcher_node, regexp|


maybe you can combine the 2 matchers in one and just check if the method_name of the matcher_node is match and argument is of regexp_type

Firstly they was combined, but it was split according to below comment:
#641 (review)

I guess my comment “that would reduce duplication in the matchers, but perhaps introduce extra duplication further down in the class…” turned out to be true :-)

Darhazer · 2018-11-01T15:42:44Z

@ypresto would you have time to push this forward or do you prefer if I apply my own feedback on the code? It's a really nice feature and it would be great to include it in the next release

ypresto · 2018-11-02T07:34:32Z

updated PR!

Darhazer · 2018-11-05T09:11:27Z

@bquorning maybe take another look?

bquorning

I’m fine with this change. As commented there is a tradeoff between having duplicated logic in the autocorrect method or in the node matchers, but I think it’s ok as is now.

bquorning · 2018-11-06T12:08:06Z

lib/rubocop/cop/rspec/capybara/current_path_expectation.rb

+              as_is_matcher(node.parent) do |to_sym, matcher_node|
+                rewrite_expectation(corrector, node, to_sym, matcher_node)
+              end
+              regexp_str_matcher(node.parent) do |to_sym, matcher_node, regexp|


I guess my comment “that would reduce duplication in the matchers, but perhaps introduce extra duplication further down in the class…” turned out to be true :-)

Darhazer · 2018-11-06T12:17:38Z

@ypresto please rebase 📦

ypresto · 2018-11-06T13:49:24Z

Thank you all for reviewing!
@Darhazer just rebased!

ypresto force-pushed the current-path-autocorrect branch 4 times, most recently from 5ceba73 to 82d458d Compare June 13, 2018 14:05

bquorning reviewed Jun 16, 2018

View reviewed changes

ypresto force-pushed the current-path-autocorrect branch 3 times, most recently from 11ecfe7 to 06c1a94 Compare August 2, 2018 02:04

bquorning approved these changes Aug 12, 2018

View reviewed changes

Darhazer requested changes Aug 13, 2018

View reviewed changes

ypresto force-pushed the current-path-autocorrect branch from 06c1a94 to 0f87439 Compare November 2, 2018 07:34

Darhazer approved these changes Nov 5, 2018

View reviewed changes

bquorning approved these changes Nov 6, 2018

View reviewed changes

Implement autocorrect for Capybara/CurrentPathExpectation

63a09e3

ypresto force-pushed the current-path-autocorrect branch from 0f87439 to 63a09e3 Compare November 6, 2018 13:46

bquorning merged commit 459adf5 into rubocop:master Nov 6, 2018

nunosilva800 mentioned this pull request Aug 5, 2019

Auto-correct of offence Capybara/CurrentPathExpectation is not safe #798

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement autocorrect for Capybara/CurrentPathExpectation #641

Implement autocorrect for Capybara/CurrentPathExpectation #641

ypresto commented Jun 13, 2018 •

edited

ypresto commented Jun 13, 2018 •

edited

bquorning left a comment

bquorning Jun 15, 2018

ypresto Aug 2, 2018

Darhazer Aug 13, 2018

bquorning Jun 15, 2018

ypresto commented Jun 25, 2018 •

edited

ypresto commented Aug 2, 2018

bquorning left a comment

Darhazer Aug 13, 2018

Darhazer Aug 13, 2018

Darhazer Aug 13, 2018

Darhazer Aug 13, 2018

ypresto Nov 2, 2018

bquorning Nov 6, 2018

Darhazer commented Nov 1, 2018

ypresto commented Nov 2, 2018

Darhazer commented Nov 5, 2018

bquorning left a comment

bquorning Nov 6, 2018

Darhazer commented Nov 6, 2018

ypresto commented Nov 6, 2018

Implement autocorrect for Capybara/CurrentPathExpectation #641

Implement autocorrect for Capybara/CurrentPathExpectation #641

Conversation

ypresto commented Jun 13, 2018 • edited

ypresto commented Jun 13, 2018 • edited

bquorning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypresto commented Jun 25, 2018 • edited

ypresto commented Aug 2, 2018

bquorning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darhazer commented Nov 1, 2018

ypresto commented Nov 2, 2018

Darhazer commented Nov 5, 2018

bquorning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darhazer commented Nov 6, 2018

ypresto commented Nov 6, 2018

ypresto commented Jun 13, 2018 •

edited

ypresto commented Jun 13, 2018 •

edited

ypresto commented Jun 25, 2018 •

edited