Rubocop-rspec: Cop idea: Don't allow `visible: true` or `visible: false` for capybara matchers

[aried3r]

When trying to match a selector, you can use the visible: finder option. However this might have unexpected results, for example visible: false does not look for invisible elements, but visible and invisible elements.

expect(page).to have_selector('.my_element')
do_something_that_should_hide_my_element
# This would succeed, even if `.my_element` is still visible.
expect(page).to have_selector('.my_element', visible: false)

expect(page).to have_selector('.my_element')
do_something_that_should_hide_my_element
# This would fail if `.my_element` is still visible.
expect(page).to have_selector('.my_element', visible: :hidden)

Happy to discuss :)

[pirj]

Sounds reasonable. Would you like to hack on it?

[aried3r]

Hey! Yes, I'd love to try. Are there any similar cops that I could use as an example? I've never written a cop myself so far so any help is appreciated.

[pirj]

Pick any simplest as a template (I believe there even might be a generator for new cops, check rake -T).
Use something like:

def_node_matcher :false_have_selector?, '(send :have_selector _ (hash <(pair (sym :visible) false) ...>))'

def on_send(node)
  add_offense(node) if false_have_selector?
end

And then add as many different examples to see that valid usages are ignored and visible: false ones are flagged.
Please don't hesitate to push your work in progress and ask any questions along your way.

[clmay]

So, this was added as a default cop, but was there discussion with any other Capybara users before doing so?

The Capybara API behaves the way it behaves for a reason, and its users rely on being able to use it as it was _designed_ and _meant to be used_ (i.e. visible: true finds only visible elements, and visible: false finds any elements irrespective of visibility on page) without it randomly becoming a lint for which they now have to add an exception to their linter config.

I don't think Rubocop should be in the business of policing 3rd party library usage just because someone doesn't understand the design of that library. This is peevesome, and I don't see why some user can propose codifying a personal preference in a default cop without soliciting input from the wider community whose practice is affected by it.

[clmay]

I just updated my Rubocop version through whatever release included this change, and now I am confronted with either:

1. Updating the visible: usage across dozens of large projects used at my company, for no real benefit
2. Updating the Rubocop configuration across these dozen projects to disable what should not have been a default cop in the first place
3. Updating the organization-wide Rubocop config from which the project configs inherit (and all the attendant change management that it will entail)

And for what? To appease the sensibilities of some rando?

Is there not any kind of consensus process required before enabling a cop as default in a release? It's one thing to make cops _available_ for those who want them. Another thing entirely to proceed the other way.

In the last view version of Rubocop I've upgraded through, there have been on the order of 1 dozen or so default cops added, with all the attendant warnings about config this or that... Do I now need to review all of them one by one to see whether or not there's any real community interest in these cops?

[pirj]

Dear @clmay,

I sincerely appreciate the constructive part of your input.
Let's discuss, and as a possible outcome, we can consider turning the cop off by default in the next minor version.

However, ‎please keep in mind several things.

This software is free to use, but according to its license, it comes with no liability nor warranty.

We encourage users to tweak settings to fit their project style. I haven't seen any single project using the defaults without amendments.

It is created, maintained, and gets contributions from volunteers.

It is time-consuming to write cops.

This effort is not comparable to the effort of tuning its configuration.

It is not perfect, bug-free nor feature-complete.

It does not fully comply with the style guides. Those style community style guides created and maintained by volunteers. Even though there is an aspiration to harmonize the default settings with style guides, this is nowhere near complete.

There is usually no consensus around guidelines (toilet paper roll orientation is one amongst three major reasons of divorces).

Starting with RuboCop 1.0 that is about to happen, a new versioning policy comes into play, and users will be asked explicitly to enable newly introduced cops and review changed default settings. This will be happening on major releases, approximately once a year. RuboCop RSpec is going to adopt this policy.

We have an idea to provide cops, but push the configuration to third-party tools like Pundit, Capybara etc. Probably even RSpec itself

With all that said, please open a new ticket and let's start from a blank slate and discuss the defaults. I highly appreciate if you could involve Capybara's maintainers in this discussion for us to have a solid opinion.

[aried3r]

I highly appreciate if you could involve Capybara's maintainers in this discussion for us to have a solid opinion.

On that matter, Capybara's maintainer recently contributed to this cop and extended its functionality in https://github.com/rubocop-hq/rubocop-rspec/pull/909. This is not to say they necessarily agree with it being a default cop.