Introduce LeadingLet cop

[kykyi]

This PR introduces a new cop: RSpec/LeadingLet. LeadingLet has grown out of a desire to place all let and let! above other blocks in spec setup in order use these as the basic setup, and more specific blocks such as subject closer to the action (i.e. the spec they are the subject for!)

This PR leverages heavily off of the LeadingSubject class, thank you to the folks behind that one. The reason is because the logic is the same, put let above all else.

---

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.
• [x] The cop is configured as Enabled: pending in config/default.yml.
• [x] The cop documents examples of good and bad code.
• [x] 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:

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

[pirj]

Thanks for the contribution.

It might be a good idea to implement a more generic cop that would enforce the order of elements inside an example group. Something to look at is this cop https://docs.rubocop.org/rubocop/cops_layout.html#layoutclassstructure It might be problematic, though, since context and describe are used interchangeably, and are often interleaved.‎ Not sure if that other cop would support this.

Anyway, let me take a closer look over the weekend.

Don't worry about CI failure if rspec and rubocop mostly pass for you locally.

[pirj]

Do you mind turning on "allow edits from maintainers"?

I can push the fix for config validation. Or feel free to commit this patch:

From 6e2c9b6395ab9532dce36a112d280cdb40ac496e Mon Sep 17 00:00:00 2001
From: Phil Pirozhkov <[email protected]>
Date: Fri, 27 Aug 2021 23:08:38 +0300
Subject: [PATCH 1/2] fixup! Align cop description

---
 config/default.yml                   | 2 +-
 lib/rubocop/cop/rspec/leading_let.rb | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/config/default.yml b/config/default.yml
index 54757ed4..342c8344 100644
--- a/config/default.yml
+++ b/config/default.yml
@@ -430,7 +430,7 @@ RSpec/IteratedExpectation:
   StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/IteratedExpectation
 
 RSpec/LeadingLet:
-  Description: Enforce that let definitions come before all other definitions in the test.
+  Description: Enforce that let definitions come before all other definitions.
   Enabled: pending
   VersionAdded: 2.4.0
   StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/LeadingLet
diff --git a/lib/rubocop/cop/rspec/leading_let.rb b/lib/rubocop/cop/rspec/leading_let.rb
index 21f1fd97..fd2d1b15 100644
--- a/lib/rubocop/cop/rspec/leading_let.rb
+++ b/lib/rubocop/cop/rspec/leading_let.rb
@@ -3,7 +3,7 @@
 module RuboCop
   module Cop
     module RSpec
-      # Enforce that let definitions come before subject in the test.
+      # Enforce that let definitions come before all other definitions.
       #
       # @example
       #   # bad
-- 
2.32.0


From e7b50dfdaaafb7193acf682aa9e903eb1f971d49 Mon Sep 17 00:00:00 2001
From: Phil Pirozhkov <[email protected]>
Date: Fri, 27 Aug 2021 23:09:02 +0300
Subject: [PATCH 2/2] fixup! Generate cop docs

---
 docs/modules/ROOT/pages/cops_rspec.adoc | 47 +++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/docs/modules/ROOT/pages/cops_rspec.adoc b/docs/modules/ROOT/pages/cops_rspec.adoc
index 1d19e8ab..f35b3542 100644
--- a/docs/modules/ROOT/pages/cops_rspec.adoc
+++ b/docs/modules/ROOT/pages/cops_rspec.adoc
@@ -2161,6 +2161,53 @@ end
 
 * https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/IteratedExpectation
 
+== RSpec/LeadingLet
+
+|===
+| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+
+| Pending
+| Yes
+| Yes
+| 2.4.0
+| -
+|===
+
+Enforce that let definitions come before all other definitions.
+
+=== Examples
+
+[source,ruby]
+----
+# bad
+  subject { described_class.new(params) }
+  let(:params) { blah }
+
+  before { do_something }
+  let(:params) { blah }
+
+  it { expect_something }
+  let(:params) { blah }
+  it { expect_something_else }
+
+# good
+  let(:params) { blah }
+  subject { described_class.new(params) }
+
+# good
+  let(:params) { blah }
+  before { do_something }
+
+# good
+  let(:params) { blah }
+  it { expect_something }
+  it { expect_something_else }
+----
+
+=== References
+
+* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/LeadingLet
+
 == RSpec/LeadingSubject
 
 |===
-- 
2.32.0

[kykyi]

@pirj I like your spec structure suggestion!

I guess having a spec for LeadingSubject and LeadingLet is an indication that what is desired is a cop where we will be able to specify spec structure, at least for let, let!, subject, before, and after - omitting describe/context because as you said they are used interchangeably.

I might shelf this PR and put some time into a Layout/SpecStructure?

[kykyi]

allow edits by maintainers is on 👍 feel free to push commits

[pirj]

I might shelf this PR and put some time into a Layout/SpecStructure?

That would be great! I mean, this cop is still good, but if you're up for a bigger one, that's fantastic.

Actually, Layout/ClassStructure is smart enough to split definition types to categories, and set the desired expected order of those categories. This means that context/describe and other example group definitions can be put into the same category, and you may interleave them as you like.

I'd say https://github.com/rubocop/rubocop/blob/master/lib/rubocop/cop/layout/class_structure.rb can contain a lot of what you'll need. Please don't hesitate to extract this to a module in rubocop, and we'll bump the version so that the module is present. Or, just copy and paste, and we DRY it out later (however, there's a "100 lines max" limit in our rubocop.yml to my best memory).

[pirj]

I still can't push to your branch for some reason. rake generate_cops_documentation is all that's needed to fix the build.

[kykyi]

@pirj would still like to get this one in so my team can start using it, and in the meantime will work on the SpecLayout PR 🙏

I have updated from all of your comments and run the rake task too

[pirj]

To make the CI green:

For some weird reason, I still can't push.

[pirj]

get this one in so my team can start using it, and in the meantime will work on the SpecLayout PR

Makes perfect sense 👍

[Darhazer]

This conflicts with LeadingSubject. At the very least, those two should have autocorrect_incompatible_with declared, and one should be disabled by default.

It would be best to have a cop to force the structure, as @pirj suggested (and I'm quite sure we used to have such an issue open, though I can't find it), or perhaps we can have one cop (LeadingDeclaration?) that would force what comes first (let/subject)

[pirj]

Now, when I think about this cop, which basically suggests putting lets before subject.

# good
let(:params) { blah }
subject { described_class.new(params) }

The cop goes against the Leading Subject guideline:

When subject is used, it should be the first declaration in the example group.

# good
describe Article do
  subject { FactoryBot.create(:some_article) }
  let(:user) { FactoryBot.create(:user) }

The project goals state that:

Enforce the guidelines and best practices outlined in the community RSpec style guide

We have RSpec/LetBeforeExamples cop that would put lets to the top of the example group, but below subject, and below hooks.

With RSpec/LeadingSubject they would allow ordering that meets the style guide while still leaving some space for flexibility of ordering lets and hooks.

To be fair, this cop would allow putting lets before hooks, and I'm all for this style, but this is not mentioned in the style guide.

Still, I'm more inclined to close this. My sincere apologies for wasting your time and effort, @kykyi.