A bit of general feedback

[bbatsov]

I've been going over the code here because of rubocop/rubocop#13792, and while it's pretty simple overall I'm wondering about a couple of things:

• Why have separate About, Rules and Context classes? To me it seems that something like a single plugin Manifest (or whatever will be enough) I also find the names Rules and Context to be somewhat confusing, as I'm having a hard-time mapping them to what they actually carry in terms of information. I'm guessing at version 1.0+ you might not be willing to restructure those, but at least they can get more documentation as now it's just a listing of example values from which one has to infer the purpose of each class. (and the example in the README)
• Does the concept of "homepage" make sense in the About class? If something is a gem it already has it, and if it's not - it's like the same repo. In general the whole About concept seems a bit redundant to me, when dealing with gems, but I assume it exists mostly for the case when you don't.

In this example:

# `context' is an instance of LintRoller::Context provided by the runner
    def rules(context)
      LintRoller::Rules.new(
        type: :path,
        config_format: :rubocop,
        value: Pathname.new(__dir__).join("../../config/default.yml")
      )

It'd be nice if something was actually extracted from the context, so it's clearer why it's passed there at all. Also this example really made me question the choice of the name Rules. :-)

• What's the purpose of the user configuration that can be passed the plugins? If the plugins have some internal configuration format related to their runners it's not very obvious what the additional configuration should be used for. I'm guessing some general flexibility is not a bad idea in the general, but I'm struggling to come up with the use-cases for this off the top of my head.

Anyways, this is the type of discussion that I'd love to have had while the API was being designed. I don't see any fundamental issues and I like the overall idea is fine, but it would have been nice to be able to provide some input before the API was finalized if you expected RuboCop to adopt it.

//cc @koic @dvandersluis

[koic]

Here are the insights and impressions I gained from implementing RuboCop plugin-compatible:

Granularity of Classes

I recognize the necessity of separating at least Plugin and Context classes.

Plugin class is responsible for setting its own information, such as the extension product name, version, and configuration relative path (e.g., config/default.yml), for external third-party gems like rubocop-performance.

On the other hand, Context class is defined by the runner side, such as RuboCop and Standard Ruby, and carries information set by them. Therefore, I think these classes need to remain separate.

Next, About and Rules belong to Plugin, and whether to integrate them into the Plugin class is a design choice, I image. This distinction is maybe also reflected that About and Rules are implemented as Struct instead of class.

About Information

There's a valid point that retrieving information from the gem as default values could be a smarter approach. There might be cases where overriding those values is necessary, but when I tried pluginfy rubocop-performance, rubocop-rails, and rubocop-minitest, the information being set was essentially the same. In most cases, using the default settings form gemspec would likely suffice.

A part from that, I love the approach where the product information of external cop is managed by plugin side, without RuboCop itself having to manage this extenal product information :-)

Naming

I don’t feel much discomfort with the name Context, but given that the term runner_context inside Standard Ruby is also used, it might be worth considering a more descriptive or colorful name (or potentially an even better alternative). That being said, Context is also one simple option.

The only name I found slightly challenging to grasp was Rules.

[bbatsov]

I don’t feel much discomfort with the name Context, but given that the term runner_context inside Standard Ruby is also used, it might be worth considering a more descriptive or colorful name (or potentially an even better alternative). That being said, Context is also one simple option.

Yeah, I assume the naming is somehow connected to Standard, which is fine. I'd probably have named this something like Runtime/PlatformContext or Platform/RuntimeMetadata. I'm assuming that most like each runner will usually support just one engine I'm wondering if there's the need to separate between runners and engines in general. (basically it's seems to me that all runners for some engine should be able to run the plugins for it, but I'm guessing the idea here is to be able to filter out RuboCop plugins when using Standard and vice-versa) Still, I'm not sure if in some cases end users wouldn't like to mix those somehow.

The only name I found slightly challenging to grasp was Rules.

Yeah, that's definitely the name that confused me the most. I hope we'll be able to at least alias it to something else more descriptive down the road.

There's a valid point that retrieving information from the gem as default values could be a smarter approach.

Yeah, I've been thinking it'd be nice to be able to instruct it somehow to use the gemspec data and just override specific keys if needed. I was also thinking we can add a :source_code key for the cases where a plugin doesn't have dedicated repo or homepage. Basically, mimic the structure from the gemspec.

Anyways, nothing major - just random small ideas.

[searls]

Hey @bbatsov, thanks for kicking the tires and reading through everything. For better or worse (though, as an API declaration library, probably the better), I haven't really looked at any of this code in almost two years, so I won't pretend to remember where my head was at when I wrote it all. Because Standard was the first customer of lint_roller, a good amount of the things I encoded into it were a hand-wavy/theoretical future-proofing (in the hope RuboCop and other quality tools might later adopt it). That said, I was pretty careful to avoid encoding any assumptions/associations between Standard or RuboCop and lint_roller itself, so I'm fairly sure things like the name Context are only incidentally used in both places.

• Why have separate About, Rules and Context classes? To me it seems that something like a single plugin Manifest (or whatever will be enough) I also find the names Rules and Context to be somewhat confusing, as I'm having a hard-time mapping them to what they actually carry in terms of information. I'm guessing at version 1.0+ you might not be willing to restructure those, but at least they can get more documentation as now it's just a listing of example values from which one has to infer the purpose of each class. (and the example in the README)

• Does the concept of "homepage" make sense in the About class? If something is a gem it already has it, and if it's not - it's like the same repo. In general the whole About concept seems a bit redundant to me, when dealing with gems, but I assume it exists mostly for the case when you don't.

Reading through the code now, I'd say the reason for this is:

1. The Context is instantiated by the runner to describe to each lint_roller plugin the context under which it plans to invoke the rules provided by the plugin so that the plugin can tailor that ruleset given said context. For example, RuboCop passes plugins a Context with engine and rule_format both set to :rubocop. But if, say, Brakeman were to add lint_roller support, it would pass a context to potentially the very same plugin with an engine of :brakeman or similar.
2. In turn, the plugin is responsible for instantiating and returning Rules to the engine, which will surely vary dramatically based on the provided Context. That is to say, if I'm a Sorbet lint_roller plugin and someone invokes rules(context) on me, I could respond differently based on the value of :engine or :rule_format or what have you. And those rules might be stored in a YAML file or generated on-the-fly as a hash, or whatever other proprietary format some other imagined engine might demand at a given path. Some sort of specification format needs to exist and I wanted to support both static files as well as dynamically-generated rules, depending on the context and plugin configuration
3. The About class exists separate and apart from the gemspec, because lint_roller plugins need not be gems, in the same way a lot of large orgs have in-house RuboCop style configurations that they distribute in a git repo / submodule / monorepo path. In those cases, leaning on gem-specific metadata would prevent a custom ruleset first being organized as a plain Ruby plugin class before being distributed as a gem. So many organizations manage their own rulesets without publishing gems of those rules that I wanted to be very explicit about having lint_roller plugins be able to describe themselves without assuming they'd be gems. One could, of course, write a plugin to fill in the About information using their own gemspec's metadata if they wanted to DRY it up

• What's the purpose of the user configuration that can be passed the plugins? If the plugins have some internal configuration format related to their runners it's not very obvious what the additional configuration should be used for. I'm guessing some general flexibility is not a bad idea in the general, but I'm struggling to come up with the use-cases for this off the top of my head.

One example that's very useful is when a configuration should differ based on the version of a technology that's affiliated with the plugin. the standard-sorbet plugin could have a different ruleset for one version of Sorbet versus another. The standard-rails plugin does do exactly this