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

Add support for enforcing symbolized shared examples #1746

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

jessieay
Copy link
Contributor

@jessieay jessieay commented Nov 22, 2023

  • Existing check only allows to enforce one style, which is called "titelized" in the error message but really just looks for a string type.
  • Enforcing the existing style raises an error when a shared example uses a symbol definition rather than a string.
  • But there are some benefits to using symbol types instead, as is discussed here: https://gitlab.com/gitlab-org/gitlab/-/issues/427697
  • As a result, some codebases might want to enforce a symbol type instead.

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.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md 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).

If you have created a new cop:

  • Added the new cop to config/default.yml.
  • The cop is configured as Enabled: pending in config/default.yml.
  • The cop is configured as Enabled: true in .rubocop.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.
  • Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

  • Set VersionChanged: "<<next>>" in config/default.yml.

@jessieay jessieay force-pushed the master branch 2 times, most recently from a91e169 to 60ce214 Compare November 22, 2023 07:27
@jessieay jessieay changed the title Add support for enforced symbolized shared examples Add support for enforcing symbolized shared examples Nov 22, 2023
@jessieay
Copy link
Contributor Author

jessieay commented Nov 22, 2023

Oh interesting, I ran rake locally and it passed/had no docs output but CI confirmed that I hadn't run rake generate_cops_documentation - I guess that isn't part of the default Rake task. Maybe it should be (or should be called out in the PR template)

@pirj
Copy link
Member

pirj commented Nov 22, 2023

It is documented, so, my apologies, you don’t get those hipster creds 😄

The arguments that deficiencies of an IDE causing cognitive resistance and thus degraded code quality don’t appeal to me as sufficient justification to change the code notation from more human-readable to more machine-readable. Especially when it comes to RSpec that is biased towards human-readable.

But here, in rubocop, as opposed to a more black-and-white style guides, we accept cops and styles to accomodate major styles, and GitLab source can certainly be considered to be one of the major styles.

Unless given my arguments above you’d love to reconsider your decision internally, I’ll be happy to review and accept this.

Thanks for your contribution, and we’ll be happy if you decide to upstream your other RSpec-targeted cops from the GitLab source to us.

@pirj
Copy link
Member

pirj commented Nov 22, 2023

It makes sense to ‘rm Gemfile.lock && bundle’ to ensure your local is on par with CI.
Also just run that rake task to see if code and generated docs are in sync. It is part of the default rake task, but things happen 🤷

@jessieay
Copy link
Contributor Author

It is documented, so, my apologies, you don’t get those hipster creds 😄

The arguments that deficiencies of an IDE causing cognitive resistance and thus degraded code quality don’t appeal to me as sufficient justification to change the code notation from more human-readable to more machine-readable. Especially when it comes to RSpec that is biased towards human-readable.

But here, in rubocop, as opposed to a more black-and-white style guides, we accept cops and styles to accomodate major styles, and GitLab source can certainly be considered to be one of the major styles.

Unless given my arguments above you’d love to reconsider your decision internally, I’ll be happy to review and accept this.

Thanks for your contribution, and we’ll be happy if you decide to upstream your other RSpec-targeted cops from the GitLab source to us.

Thanks for your thoughts @pirj !

Even if GitLab chooses to continue enforcing string descriptions for shared examples, I think it makes sense to provide this configuration as an option. Like you said, rubocop can and should accommodate different styles. :)

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thank you!

Looks mostly good.
The main thing to fix isvthe indentation inside heredoc in specs.

I apologize for the state of the spec, and that it has examples of shared_example usage with unacceptable arguments. I hope you feel like to fix this along the way.

lib/rubocop/cop/rspec/shared_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/shared_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/shared_examples.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/shared_examples.rb Show resolved Hide resolved
spec/rubocop/cop/rspec/shared_examples_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/shared_examples_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/shared_examples_spec.rb Outdated Show resolved Hide resolved
@jessieay jessieay force-pushed the master branch 2 times, most recently from c949fa1 to 842e690 Compare December 11, 2023 05:53
@jessieay
Copy link
Contributor Author

Thank you for the feedback @pirj and sorry for the delay in addressing it. I believe that I have incorporated all of your feedback. Let me know what you think!

Copy link
Collaborator

@bquorning bquorning left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution, @jessieay, and apologies for overlooking it for so long. Could you please rebase on master?

@pirj Do you have time a final review? 🙏🏼

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thank you!

* Existing check only allows to enforce one style, which is called
  "titelized" in the error message but really just looks for a string
  type.
* Enforcing the existing style raises an error when a shared example
  uses a symbol definition rather than a string.
* But there are some benefits to using symbol types instead, as is
  discussed here: https://gitlab.com/gitlab-org/gitlab/-/issues/427697
* As a result, some codebases might want to enforce a symbol type
  instead.
@bquorning
Copy link
Collaborator

Could you please rebase on master?

Sorry, I did it myself. I hope you don’t mind 😄

@bquorning bquorning merged commit 2825622 into rubocop:master Jan 4, 2024
24 checks passed
@pirj
Copy link
Member

pirj commented Jan 5, 2024

There’s a bug here, #1765

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.

4 participants