rubocop-rspec icon indicating copy to clipboard operation
rubocop-rspec copied to clipboard

Add `require_implicit` style to `RSpec/ImplicitSubject`

Open r7kamura opened this issue 2 years ago • 5 comments

Although the explicit style is recommended by default, some users may prefer the implicit style. For such users, it would be better if there is an option to make the style consistent with the explicit style, rather than asking them to disable this cop.


Before submitting the PR make sure the following are checked:

  • [x] Feature branch is up-to-date with master (if not - rebase it).
  • [x] Squashed related commits together.
  • [x] Added tests.
  • [x] Updated documentation.
  • [x] Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • [x] 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 in default/config.yml to the next minor version.

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

  • [x] Set VersionChanged in config/default.yml to the next major version.

r7kamura avatar Aug 28 '22 01:08 r7kamura

I think the name of the style "always" is debatable. Since this cop is called ImplicitSubject, I named it "always" as in "always use implicit subject." If you have any other good ideas, please let me know.

r7kamura avatar Aug 28 '22 01:08 r7kamura

I added a multi-line example and then an example about named subject.

I figured that if named subject is used, it would be better to ignore it. Or should we also flag named subject as an offense and prompt to convert to unnamed subject?

This is my opinion, but I personally think it would be better to keep this cop only to decide whether is_expected should be used or expect(subject) should be used. Rather than having this cop decide what the named subject should be, it would be better to have a separate cop that decides whether the named subject should be recommended or not, and have it decide there.

r7kamura avatar Aug 31 '22 23:08 r7kamura

I tend to agree that if there is named subject in use, it's better to be left explicit, as you can't use the name with the implicit syntax, and there might be a reason you have chosen a name.

Darhazer avatar Sep 01 '22 10:09 Darhazer

if there is named subject in use, it's better to be left explicit, as you can't use the name with the implicit syntax, and there might be a reason you have chosen a name.

Furthermore, there can be more than one:

subject(:foo) { ... }
context '' do
  subject(:bar) { ... }
  it do
    expect(foo).to ...
  end

and we'll end up with a task to properly detect if the used explicit subject is also the right implicit one.

pirj avatar Sep 01 '22 18:09 pirj

I have renamed some of the required CI checks, which unfortunately means that you will need to rebase this branch to make CI pass.

bquorning avatar Sep 12 '22 13:09 bquorning