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 configuration option OnlyStaticConstants to RSpec/DescribedClass #1825

Merged
merged 1 commit into from Mar 3, 2024

Conversation

ydah
Copy link
Member

@ydah ydah commented Mar 1, 2024

Fix: #1741 (comment)
Fix: #1741 (comment)


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).

@ydah ydah requested a review from a team as a code owner March 1, 2024 18:13
@bquorning
Copy link
Collaborator

@pirj, I would love to hear your opinion on this PR.

@pirj
Copy link
Member

pirj commented Mar 2, 2024

I see it as a code smell when constants of the class under test are referenced from its spec, at least in a way that #1741 describes.

In general, this option probably makes sense to avoid offences in examples that are less coupled with the class under test.

I don’t have experience turning on Sorbet for specs, so I can’t suggest anything on this front.

I don’t take the “our CI pipelines fail” argument given that auto-correction doesn’t break the spec.

Personally, I don’t see why ‘described_class::MyException' is any different from ‘described_class::VERSION’, but I understand that some people may prefer to use different styles for them.

Again, personally I think ‘described_class’ was a mistake, and we it’s good to quote class names in the top-level example group’s description, and to use the full class name elsewhere in the spec, with no indirection.

@bquorning
Copy link
Collaborator

Again, personally I think ‘described_class’ was a mistake, and we it’s good to quote class names in the top-level example group’s description, and to use the full class name elsewhere in the spec, with no indirection.

I don’t remember us having this discussion before. I’m not sure I find described_class to be that useful either. Do you know why it’s even a feature in RSpec?

@pirj
Copy link
Member

pirj commented Mar 2, 2024

why it’s even a feature in RSpec?

This I can’t remember, but there’s a funny story about the RSpec 3 release and described_class, also a nice compilation of quotes and opinions in a discussion below the description rubocop/rspec-style-guide#104. Since most likely the described_class will stay in RSpec 4, the cop should remain in some form, too.

If it’s not a big maintenance overhead for us to provide our users with an optional … option to style their code the way they like, why not?

@bquorning bquorning merged commit f2593cd into master Mar 3, 2024
24 checks passed
@bquorning bquorning deleted the only-static-constants branch March 3, 2024 19:32
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.

Improve RSpec/DescribedClass cop
3 participants