Disable recently problematic rules

[kevindew]

This is to follow #124 it disables two rules that prove problematic as a result of the dependency updates. This build will fail until #124 is merged and this is rebased onto it. Further details in the commits.

[benthorner]

This makes sense to me - I agree with both of the overrides and the attached explanations. Following the discussion in #124, I'm still concerned about a couple of Cops that we're leaving enabled:

• Lint/MissingSuper - I think this will lead to unnecessary boilerplate, and is really a testing issue. I don't see the harm in disabling it, but I can see the irritation of applying it, especially if we continue reusing patterns like ApplicationService in other repos. Can you think of a benefit to applying it, that wouldn't be covered in testing?

• Lint/UselessMethodDefinition - this one sounds beneficial on the surface, but is actually an issue for Rails controller actions: removing the "useless" method definition will lead to an error. A compromise here might be to disable it for app/controllers specifically, or maybe more specifically in the affected repos / controllers.

[kevindew]

Thanks @benthorner for taking a look and for your help with this 👍 It's good that we're down to only a couple of divisive points in the world of linting.

For both the rules you referenced I don't have particularly strong opinions for or against them myself (like many rules can see the occasional benefits and then times of annoyance). I therefore find myself leaning slightly more towards allowing them and seeing if they cause problems as the divergence of disabling rules is something we carry for a long time.

Lint/MissingSuper - I think this will lead to unnecessary boilerplate, and is really a testing issue. I don't see the harm in disabling it, but I can see the irritation of applying it, especially if we continue reusing patterns like ApplicationService in other repos. Can you think of a benefit to applying it, that wouldn't be covered in testing?

From @theseanything's stats it seems like this isn't too widespread a problem - mostly one we've introduced relatively recently into Content Publisher and Email Alert API. I can certainly see why it'd be annoying for those. However since it's so low widespread there is a part of me that wonders: are we actually coding in a strange way to hit this problem? I guess what we're doing that is odd is using the subclass as the place to define an interface that isn't on the superclass - this seems a violation of Liskov substitution principle but then this isn't the most Rubyist of principles - similar things exist in Sidekiq workers for instance with the perform interface. Perhaps though, we've used direct inheritance when a mixin is more appropriate for the type of behaviour that is shared?

Lint/UselessMethodDefinition - this one sounds beneficial on the surface, but is actually an issue for Rails controller actions: removing the "useless" method definition will lead to an error. A compromise here might be to disable it for app/controllers specifically, or maybe more specifically in the affected repos / controllers.

Sure. I can't quite see why that'd cause a Rails error (could you explain?) however I can see that this causes an annoyance when considered with Rails/LexicallyScopedActionFilter (as per commits like alphagov/whitehall@7fd5fa0) - so it seems, for Whitehall at least, that we can only have one of these enabled - I'm not sure the impact on other apps. The fact that this rule is auto-correctable isn't much help if it just triggers a different one in one place. I'm not sure the best course of action to take, I'm somewhat hopeful only Whitehall is particularly grim with the mix of inheritance with different before_action 😬 so might be able to dealt with in just that repo.

You also mentioned in your comment:

more generally how we're going to apply this update so repos don't get left behind.

Yup - this is a tricky problem. I see two aspects to it - the initial dropping of this change to see if any of the rules are really miserable and then the long tail of getting it everywhere. I'm wondering whether it might be worth doing a pre-release of this gem with the new rules and applying them to a bunch of our key repos first (with a little call for volunteers to help) to iron out the first step. Second step would probably involve needing a few volunteers again, I somewhat imagine that since I'm involved in the release I'll have a drive to make it happen everywhere.

[benthorner]

Hmm...I'm in still in two minds with Lint/MissingSuper. It's not auto-correctable, so I'm leaning towards us pushing ourselves to question its value: what do we gain by having this rule? The Liskov principle isn't something I'm familiar with, and looking at it I agree it's not really for Ruby, which doesn't have a formal way to define "interfaces".

are we actually coding in a strange way to hit this problem?

This gave me some pause for thought. I think patterns like this (for anyone else reading) are OK, though:

• It is true to say that ServiceX is a ApplicationService.
• It is conventional to initialise state in the constructor.
• (Precedent: everything inherits from Object, yet doesn't call super.)

I believe the pattern we have is the most minimal to achieve the desired, class-level interface, which I take as a sign that it's the "best" or "right" one for the job. So I'd only expect its usage to grow. Unless we can think of an advantage over testing, then it seems in our interest to disable the rule. Ruby isn't meant to be a boilerplate-y language, after all.

Sure. I can't quite see why that'd cause a Rails error (could you explain?)

Ahh, I misremembered this, sorry. It was actually that Rails/LexicallyScopedActionFilter encourages you to create empty methods that break the code unless they call the superclass method explicitly. So removing them isn't a problem. Both of them are quite circumstantial, so it's hard to choose between them.

I think it's important we promote a good experience with this repo. Even one instance of this conflict would be quite a poor experience for the developer trying to figure out how to resolve it. On that basis, I think we should scope Lint/UselessMethodDefinition to exclude app/controllers, where it's unlikely to apply anyway.

That was long. Here's a quick summary of my thoughts so far:

• Lint/MissingSuper - will entail manual work for unclear benefit. I think we need a clear argument for it.
• Lint/UselessMethodDefinition - restrict to exclude app/controllers, to avoid current and future conflicts.

I also like your idea of a pre-release, which nicely captures the potential compatibility issues that may crop up when applying all the changes. Perhaps @theseanything you'd like to comment here. What's your take on these Cops?

[kevindew]

The Liskov principle isn't something I'm familiar with, and looking at it I agree it's not really for Ruby, which doesn't have a formal way to define "interfaces".

Ah sure, it's the L in SOLID. It's a principle applied to Ruby frequently so I may have mis-represented it as saying it's not the most Rubyists of rules, something to aspire too but not dogmatic.

are we actually coding in a strange way to hit this problem?

This gave me some pause for thought. I think patterns like this (for anyone else reading) are OK, though:

• It is true to say that ServiceX is a ApplicationService.
• It is conventional to initialise state in the constructor.
• (Precedent: everything inherits from Object, yet doesn't call super.)

I believe the pattern we have is the most minimal to achieve the desired, class-level interface, which I take as a sign that it's the "best" or "right" one for the job. So I'd only expect its usage to grow. Unless we can think of an advantage over testing, then it seems in our interest to disable the rule. Ruby isn't meant to be a boilerplate-y language, after all.

Sure, I'd have thought it could be similarly minimal as a mixin? I guess the two parts that make me question what we've done here is 1) it seems to be a violation only affecting new(ish) code rather than a wider GOV.UK precedent? and 2) the question asked about this rule is (paraphrased) "what is a good reason to not call super?" and the only answer I've got for this example is that it's not really a class we're inheriting from it's more a behavioural pattern, as we'd not expected the class to actually do anything.

Happy to be led by others input into this - I know this has become a very specific discussion 😄

Sure. I can't quite see why that'd cause a Rails error (could you explain?)

Ahh, I misremembered this, sorry. It was actually that Rails/LexicallyScopedActionFilter encourages you to create empty methods that break the code unless they call the superclass method explicitly. So removing them isn't a problem. Both of them are quite circumstantial, so it's hard to choose between them.

I think it's important we promote a good experience with this repo. Even one instance of this conflict would be quite a poor experience for the developer trying to figure out how to resolve it. On that basis, I think we should scope Lint/UselessMethodDefinition to exclude app/controllers, where it's unlikely to apply anyway.

Ok great about the errors. I kind of feel if we wanted to have an exception it'd be better to disable Rails/LexicallyScopedActionFilter rather than conditionally disabling Lint/UselessMethodDefinition to a particular directory - that feels a little less surprising than a rule only applying to certain directories.

I'm worried though that this talk of disabling is almost entirely driven by Whitehall where the code affecting this is already pretty whacky - where it'd seem reasonable to do an inline ignore on the offending controllers. Do we know where the other cases of this are?

That was long. Here's a quick summary of my thoughts so far:

• Lint/MissingSuper - will entail manual work for unclear benefit. I think we need a clear argument for it.
• Lint/UselessMethodDefinition - restrict to exclude app/controllers, to avoid current and future conflicts.

I also like your idea of a pre-release, which nicely captures the potential compatibility issues that may crop up when applying all the changes. Perhaps @theseanything you'd like to comment here. What's your take on these Cops?

Great, thank you - I'll try plug some other people to take a look at this tomorrow too.

[benthorner]

I kind of feel if we wanted to have an exception it'd be better to disable Rails/LexicallyScopedActionFilter [...] Do we know where the other cases of this are?

Yes, after writing this I realised I was biased towards Whitehall. Looking at it that way, I agree the inheritance structure in that repo is unusual and not something we'd want to replicate elsewhere. So it makes sense to do something specifically in Whitehall, as I'm not aware we have filtered, inherited controller actions anywhere else. Both of the rules are quite niche, but the lexical one has the benefit of being more directly related to the code, so easier to explain disabling it.

So I'd agree with you there, well argued!

it seems to be a violation only affecting new(ish) code rather than a wider GOV.UK precedent?

(Facetious, but the wider GOV.UK precedent here appears to be "a mess", so I'm not particularly swayed by this 😸.)

Sure, I'd have thought it could be similarly minimal as a mixin?

I think it would be slightly more verbose. The actual problem seems to be withinitialize, so with a mixin or with inheritance we'd end up having to define some other setup method to avoid the Cop. This is what I find pointless: it doesn't make sense to me to have to initialize a service with nothing, and then have to call another method to do the equivalent. Arguably the Cop is just unintelligent: the calls to super are pointless, but it doesn't do enough checking to see that.

I think this really comes down to a decision: do we want to put super in lots of places it's not needed, or rewrite the existing patterns more verbosely so the Cop arbitrarily doesn't notice them, or disable the Cop for those classes?

[theseanything]

+1 on the idea about pre-releases. It'd be a good way to gauge the real impact of changes.

As for Lint/MissingSuper, I think it should be enabled. As Kevin said, for this particular example we might want to consider composition rather than inheritance. For me it's quite odd, there's only a partially shared interface (the call method) and no common behaviour between sibling classes. For that particular example, could we re-write it as the following without increase verbosity?:

module Service
  def call(*args, **kwargs)
    new(*args, **kwargs).call
  end
end

class DataLoader
  extend Service

  def call
    #do_something
  end
end

I think this type of rule would help us think about our code design, i.e. we are calling super, but we don't actually need it - is inheritance actually appropriate here?

[benthorner]

@theseanything this is what discussion with @kevindew. What's difficult here is that this change affects quite a lot of files, which will need to be manually fixed. Apart from blindly adding super everywhere, which isn't really helping anyone, there doesn't seem to be a "better" alternative to what we have now. Your example is a bit too minimal:

module Service
  def call(*args, **kwargs)
    new(*args, **kwargs).call
  end

   # can't do "private_class_method :new" here :-(
end

class DataLoader
  extend Service

  def initialize(arg_i_need)
     @arg_i_need = arg_i_need
  end

  def call
    #do_something
  end
end

The use of initialize is really the crux of the pattern and the problem with this Cop: most services (or other classes using a similar pattern) need data to run. It makes sense to initialize them with this data, as this means we can avoid passing all the data around to any private methods used by call. So, if the service needs data to exist, it makes sense for it to be initialized with that data. Or, we could switch to a more ambiguous pattern, like this:

module Service
  def call(*args, **kwargs)
    new.setup(*args, **kwargs).call
  end
end

class DataLoader
  extend Service

  def setup(arg_i_need)
     @arg_i_need = arg_i_need
  end

  def call
    #do_something
  end
end

And this is what I'm against: setup is really just a synonym of initialize. They do the same thing, the only different is that with setup, RuboCop doesn't force us to write super in it.

My conclusion so far is that applying this Cop will be a net deterioration: genuine omissions of super should be caught in testing; we shouldn't rely on linting to spot bugs. So I'm keen to explore how we can configure this Cop so the end result is sensible. That seems to hinge on agreeing whether there's merit in the "Service" pattern we've got.

Can you see the tradeoff here?

[kevindew]

@benthorner If I understand right you're theorising that with a mixin you'd still need to avoid initialize, however that wouldn't be the case (I actually cobbled together a rough implementation when chatting about this the other day here: alphagov/content-publisher@7b117d7). Whereas the setup example you've defined there is how you would approach this with inheritance, as that would keep the initialize method free for the superclass to implement.

With a mixin - you tend to be adding an interface to a class rather defining the class' lifecycle. For example, using include Enumerable from stdlib you'd not expect that to be altering initialize. Whereas with a class approach from stdlib, SimpleDelegator, you'd fully expect to need to call super if you extended initialize.

I forgot the other day to shout to get other eyes on this, I'm thinking I could do that tomorrow but it might not be the most inviting of a conversations to get involved in 😆 . It may be worth trying to summarise the concerns to see if they can be tested?

[benthorner]

Sure, thanks for digging into it a bit more @kevindew. Here's a summary for anyone reading:

• We're all agreed on disabling Naming/VariableNumber and RSpec/MultipleMemoizedHelpers.
• We're all agreed on doing a pre-release of the overall v1 upgrade, so we can trial it on certain repos.
• I'd like to also consider disabling Lint/MissingSuper, as it conflicts with a pattern we have in e.g. Content Publisher.

Latest thoughts from me on Lint/MissingSuper:

• If we leave it enabled, we should have a plan to address it that's not adding super everywhere it's not necessary.
• I've since learnt the Cop can be avoided with a use of extend, and @theseanything suggested an approach.
• @kevindew refined the approach to match the current behaviour, and suggested a granular, trait-like naming for it.

That last one tips the balance for me, especially since it's clearly distinct from the current base classes ApplicationService, which are conventionally inherited from (e.g. ApplicationRecord, ApplicationController).

So, I'm for approving this PR, if we're comfortable with the new approach to avoid Lint/MissingSuper.

[kevindew]

Great thanks for that summary @benthorner.

So a quick proposal on next step actions:

• You (or someone else) approves this PR
• I do a shout in #govuk-developers to say "Anyone have concerns on the linting rules that are coming" to see if this raises concerns for anyone
• If there aren't concerns in a day I'll merge these PRs and create a PR to create the pre-release version of the gem. I'll accompany that with an issue with the pre-release for anyone to point out pain points (I'll include the concerns about Lint/MissingSuper and Lint/UselessMethodDefinition there)
• I'll then encourage some people to try it, though it might be mostly me trying it. I'll plan to delay Content Publisher and Email Alert API a bit here

Does this sound ok? and are we happy for this to be the long foretold 4.0.0 release?

[benthorner]

Yes 👍. Thanks for all your effort here @kevindew 🏅.