Remove BaseRequest

[st0012]

Motivation

When pairing with @vinistock, we found that after the listener pattern migration, none of the BaseRequest descendents use the visitor pattern anymore. So now its only usage is to define the @document ivar, which is a wasteful reason to have a class.

Implementation

1. Remove the BaseRequest class
2. Remove the obsolete override check task
3. Update check docs as now some requests don't even have a common request parent class

Automated Tests

Manual Tests

[Morriar]

So they all uses the same @document and all define run yet we do not want a common ancestor? This change doesn't make sense.

What is the drawback of keeping BaseRequest?

[st0012]

Since each request is invoked individually, it doesn't matter if they have a run, response, or a foo method for them to return the result. Having a parent class to define that interface doesn't provide benefits like duck-typing or type checking but only a consistent-ish naming between those request classes.

And IMO having the same @document also doesn't warrant a parent class because BaseRequest doesn't build any shared logic on top of it.

What is the drawback of keeping BaseRequest?

I think it gives an false-impression that those requests have as strong commonality as those inherit from Listener, which as I explained above, is not true.

Since @vinistock and I are re-thinking the structure of our requests & listeners (e.g. #1247), I think keeping this false-impression will reduce the outcome of it.

What's the benefit of keeping it?

[Morriar]

The benefit of the super class defining an abstract run is that you enforce a convention about where the "meat" of a request is implemented. This consistent-ish naming is important as it helps establish commonality between requests.

I think that instead of removing the BaseRequest concept we should instead lay on it. From what I understand the ultimate goal of this series of refactoring is untangling the mess of requests, extensible listeners and listeners.

ExtensibleListener should be removed and add-ons should instead register directly inside the related BaseRequest subclass. The dual concept between Listener vs. ExtensibleListener would then disappear and avoid confusion.

BaseRequest should thus provide all the add-on registration logic in a generic way. As an add-on implementor I only care about registering for a request and the request should then enforce the kind of implementation I should provide.

This means each subclass of BaseRequest should be responsible of defining what "kind" of add-on implementation they accept. For example Hover and friends (maybe all things that could become subclasses of a DocumentRequest actually) would expect a Listener-based add-on. Diagnostics would expect more of an Observer-based add-on etc.

Then, each subclass of BaseRequest would be responsible of triggering the right behavior in its registered add-ons.

Removing the commonality between request will make it easier for requests to stray out of the blessed path and will create a forest of somewhat identical implementations that will make it harder to maintain over the long term. Keeping this superclass, forces us to provide a clean and common implementation. It also enforces a convergent design where contributors external to your team understand how (or are forced to) provide an implementation following your philosophy.

One more change that could be done in parallel is adding a requires_ancestor { Listener } to Support::Common to avoid this module being included where it shouldn't be, making it harder to decouple later. Or maybe the Support::Common methods should be provided to the add-on through the Request or Listener itself rather than an included module.

[st0012]

As an add-on implementor I only care about registering for a request and the request should then enforce the kind of implementation I should provide.

This is indeed what I want to do with changes like this and #1247.

FWIW, we do plan to introduce a parent request class to better capture the commonality between them. And IMO removing this thin layer and not too helpful abstraction will help us come up with a better design later, either in #1247 or in a follow up PR.

[Morriar]

Okay, we talked IRL about this and we're going to accept this change as this as long as the BaseRequest concept comes back in #1247.