rubocop / rubocop-capybara

Code style checking for Capybara files.
https://docs.rubocop.org/rubocop-capybara
MIT License
41 stars 8 forks source link

Improve "Reference" (and/or rationale in docs) for existing (and future) cops #97

Closed timdiggins closed 8 months ago

timdiggins commented 9 months ago

Background: The quality of discussion on the PRs in this repo is really top notch and I've learned a lot from reading, e.g. #63 and #61. I initially started looking for the PRs because I couldn't work out what the point of Capybara/RSpec/HaveSelector was. The discussion on #63 was really great. I agree with the comment in that PR that the cops could do with a rationale - either stated within the docs of the cop, or as a Reference. Rubocop's cops mostly have references to the ruby style guide, which is where they draw their rationale.

I notice that the ["Reference")(https://github.com/rubocop/rubocop-capybara/blob/main/config/default.yml#L21) in rubocop-capybara is mostly a reference to itself. I propose that the References should be replaced with the best justification page for them (which in many cases will be the PR which introduced them).

FYI I believe (based on https://github.com/rubocop/rubocop/blob/2fe4b1a6faca23adff13e1bbff6ecf5b66c6447b/lib/rubocop/cop/message_annotator.rb#L102) there can be multiple Urls in the Reference

I think this would massively add to the value of rubocop-capybara. I will make a PR with a couple of starting points and see what people think.

timdiggins commented 9 months ago

https://github.com/rubocop/rubocop-capybara/compare/main...timdiggins:rubocop-capybara:update-references-for-two-cops

But - it seems part of the build process for the gem actually replaces these references with the rubydoc.info URL. I'm not sure the justification for that (and it might be quite a bit of work to change) so I'll wait for feedback before progressing / making a PR.

pirj commented 9 months ago

@timdiggins thanks for bringing this up. What needs to be done is really a style guide repo, rubocop/capybara-style-guide. You may have noticed StyleGuideBaseURL in that class.

Here’s an example for rubocop-rspec

Please follow this thread for the guide details. Cc @ydah @mvz.

@bquorning should we create the right styleguide repo in the rubocop namespace from the start, or is it fine if @timdiggins starts a repo, and we transfer it after?

bquorning commented 9 months ago

@pirj both solutions work for me. We’d have to set up a https://github.com/rubocop/capybara-style-guide repo, and have it publish into https://capybara.rubystyle.guide/.

~Creating a separate repo, and then transfering it makes authentication/permissions easier to start with.~ Update: I created https://github.com/rubocop/capybara-style-guide, adding @timdiggins with write access and @rubocop/rubocop-rspec as admin.

timdiggins commented 9 months ago

This is a much bigger proposition than just changing the Reference URLs of a the rules to the initial PRs! I'm not a capybara best practices expert, but I'm happy to start the ball rolling on a capybara guide. I've created a skeleton README.adoc, but am having trouble pushing to the repo (maybe because it's empty?) and can't fork (because it's empty).

Have put my proposed skeleton doc into https://github.com/rubocop/capybara-style-guide/issues/1

timdiggins commented 9 months ago

Oops - pushed the wrong branch first. Can someone with admin rights change the default branch back to main? I have put very uncontroversial parts of README in main. Then I can create PR for the initial skeleton wording (currently on https://github.com/rubocop/capybara-style-guide/tree/initial-proposal-for-structure)

(I worked out why I couldn't push, because I hadn't accepted invitation).

bquorning commented 9 months ago

Can someone with admin rights change the default branch back to main?

Done

ydah commented 8 months ago

The following repositories have been created, so let's close this issue. https://github.com/rubocop/capybara-style-guide

pirj commented 8 months ago

What you did is just awesome! Thanks a lot for making this happen, guys