Thin out lets in random spec

[pirj]

Related to rubocop/rubocop-rspec#862

To convince me, maybe someone could refactor this spec file [to reduce the number of used memoized helpers] and show me how it improves it?

---

Before submitting the PR make sure the following are checked:

• [no] Wrote good commit messages.
• [-] Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• [no] Squashed related commits together.
• Added crushed tests.
• [-] Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

[pirj]

@marcandre refactoring is done, I'm ready for the harder part, part two, "show me how it improves it".

[mockdeep]

Another possibility that occurred to me is to have a spaces helper method that accepts a count argument:

def spaces(count)
  ' ' * count
end

let(:body) { "puts '#{spaces(59)}'" }

It seems like seeing the actual count in the code might be useful. I haven't looked at any of the other tests, but if we do this sort of thing in more places it might be a good global helper.

[pirj]

Ha! I was trying to avoid defining methods to replace lets because of an argument that it's basically the same thing.

But this spaces method clearly shows how methods can be better, since they do accept parameters. I really like it.

It's possible to go even further with something like:

def body(spaces:)
  "puts '#{' ' * spaces}"
end

[mockdeep]

I was trying to figure out a good way to factor out body but wasn't able to come up with an alternative that I thought I could sell. Personally, I'd probably actually copy paste it into each test. I think some duplication can be advantageous across tests, since it makes it a lot easier to write varying test cases, and it makes each test more self-contained. That, to me, is easier to maintain and understand without scrolling up and down the file (and other files) to trace through all of the implicit dependencies.

I think our programmer's inclination to DRY up code actually works against us when it comes to tests. In tests we often want logic that changes in small ways across each test, and that makes it harder to deduplicate without making things more difficult to understand and maintain.

[mockdeep]

One sort of thing I like to think about is how hard it would be to rearrange the tests in a file. I often want to restructure the tests in a file to tell a more clear story. If the tests resist being moved around, then they're too coupled to various implicit dependencies. Thinking about rearranging the tests in this file gives me panic sweats.

[pirj]

Thinking about rearranging the tests in this file gives me panic sweats.

Indeed. And it's by far not the worst example.

[mockdeep]

This almost feels like cheating. The let is still there, it's just imported from a shared context. One other possibility that occurs to me is to have a line_length_config method:

def line_length_config(overrides = {})
  {
    'Layout/LineLength' => {
      'Enabled' => true,
      'Max' => 80,
      'AllowURI' => true,
      'IgnoreCopDirectives' => true,
      'URISchemes' => %w[http https]
    }.merge(overrides)
  }
end

let(:other_cops)  { line_length_config }
let(:other_cops)  { line_length_config('Enabled' => false) }
let(:other_cops)  { line_length_config('AllowURI' => false) }

Looking at other other_cops, it's not clear to me how many of the above settings are actually necessary. is 'Enabled' implied? And aren't the other settings the default when it is enabled? Could it just be:

let (:other_cops) { { 'Layout/LineLength' => { 'AllowURI' => false } } }

Looks like the Max default changed recently to 120, so not sure if we need to account for that.

[pirj]

It's cheating in a way, yes. But also it's just hidden one let instead of three that we had before.

Why I chose that instead of a method is that we have to specify let(:other_cops) { ... }, in every place, which is somewhat clumsy.

Got an idea though, what do you think of:

RSpec.describe RuboCop::Cop::Style::IfUnlessModifier, :config do
  default_line_length_config =
     {
        'Enabled' => true,
        'Max' => 80,
        'AllowURI' => true,
        'IgnoreCopDirectives' => true,
        'URISchemes' => %w[http https]
      }

  def self.with_line_length_config(**overrides)
    let(:other_cops) do
      { 'Layout/LineLength' => default_line_length_config.merge(overrides) }
    end
  end

  context 'when AllowURI is false' do
    with_line_length_config(allow_uri: false)
  
    expect_no_offenses(<<~RUBY)
...

I hope they don't burn me as a heretic.
PS changed default_line_length_config to be a ... variable (hope it doesn't vary much though).

[mockdeep]

I actually went the route of setting other_cops in each location that needed it. I thought that ended up being easier to understand with less indirection. In some cases I set it to {} in order to unset it, which is what we're actually doing somewhat implicitly in the existing tests.

[mockdeep]

I poked around with some of this in my own PR here.

[marcandre]

@marcandre refactoring is done, I'm ready for the harder part, part two, "show me how it improves it".

@pirj Did you actually want me to ask? I can if you want me to, but I can tell you already what I like in the PR: some changes that have nothing to do with this (like removing a "context" that's not really useful). Also, the originality of the commit messages ;-)

Otherwise I remain unconvinced that this is an improvement, and that it is worth whoever is writing this spec to worry about. The whole exercise seems as futile to me as reducing the number of local variables in a code.

[pirj]

I'm pretty happy already now when you don't mention that the spec looks significantly worse this way.
The whole point was to show that it is possible to write a spec by not using much lets. For the sake of merging a cop that would limit those usages to a certain number (5 lets allowed by default).

Otherwise, I'm ready for all the criticism. If you feel that some part of this became harder to understand/change/..., I'll be glad to hear your point on this. While working on cops/style guide I try to be as neutral as possible and accept distinct coding styles. So - no arguing included, just going to learn a new point of view.

No pushing/rushing, it's completely up to you, I will not be offended if you ignore this altogether, and I completely understand it - we're about to hit 1.0, just a few things left, and it will be beneficial to everyone.

[marcandre]

I commented on #8491, partly because your PR's diff is noisier and appear to mix different improvements that are not related to the refactoring and thus felt harder to comment on.

Still, the bulk of your PR seems to be replacing some let with a shared context:

# before
let(:ignore_cop_directives) { true }
let(:allow_uri) { true }
let(:line_length_config) do
  {
    'Enabled' => true,
    'Max' => 80,
    'AllowURI' => allow_uri,
    'IgnoreCopDirectives' => ignore_cop_directives,
    'URISchemes' => %w[http https]
  }
end
let(:other_cops) { { 'Layout/LineLength' => line_length_config } }

# after
shared_context 'with LineLength settings' do |enabled: true,
                                              allow_uri: true,
                                              ignore_cop_directives: true|
  let(:other_cops) do
  {
    'Layout/LineLength' => {
      'Enabled' => enabled,
      'Max' => 80,
      'AllowURI' => allow_uri,
      'IgnoreCopDirectives' => ignore_cop_directives,
      'URISchemes' => %w[http https]
    }
  }
end

This introduces changes like:

# before
let(:allow_uri) { false }
# after
include_context 'with LineLength settings', allow_uri: false

I feel that using shared_context remains more technically and less known. The result is also more verbose. I like that this is a case where it's unlikely to make sense to want to change one setting of LineLength and only in a sub-context want to change another one, so your shared context makes clear that one can only specify these settings all at once. As for the verbosity, it makes things more explicit, I don't have a strong opinion on if it's better or worse overall, probably more like changing four quarters to a buck. In my mind, you are actually removing one let, the line_length_config; others are just shifted to keyword arguments to your shared context. It just so happens that the metric doesn't count those....

If I was reviewing some new Cop PR and got the current version with the lets, I wouldn't think twice about that and would not have an issue with this. If I got your shared_context version, I would be thinking "wow, looks like this submitter finds it so easy to write a shared context that it was worth it here, even given the additional verbosity", and I would also have no issue with that.

In short, I would have no trouble accepting either PRs, and I consider myself a tough reviewer. If I received a PR to switch from one to the other, I'd be wondering why, oh why, is the submitter focusing on that! If doesn't change much at all, just shifting how to specify allow_uri & al. I don't feel there's one way that is clearly superior to the other.

If one has time to spend on it, there are other things that would benefit more this spec. I notice there are many instances of expect(some_very_similar_string.length).to eq 79 for example. There's the strange "/#{'a' * 37}/" and probably a few other imperfections.

But ultimately, the biggest issue by far with this spec reveals a deep problem with the Cop itself: it knows way too much about the Layout/LineLength Cop. It should not be the job of IfUnless to know the existence of an Layout/LineLength/AllowURI setting! #8290 is but one solution to this. This is the kind of problems worth focusing on, in my mind.

[pirj]

Thanks! Your time is much appreciated.

[marcandre]

My pleasure.

Maybe you did already, but did you say how much of an improvement you think the shared_context is?