Mechanism to provide arguments to rules

[jacobotb]

What problem are you trying to solve?

Some rules should be configurable. For example: some repositories might want to have a rule that alerts if a function is larger than 100 lines, while others may want to set the threshold at 200.

What is your solution?

Arguments can be specified in the configuration file and they are plumbed over to the rule matcher code. The arguments that were set by the user are available in node.context.arguments as an object that's keyed by argument name.

Alternatives considered

What the reviewer should know

[jasonforal]

Larger picture question: are rule arguments conceptually any different than ignore/only?

The conceptual idea around ignore/only is that for a given path, we want to attach arbitrary metadata. Initially, we used this to store what are effectively boolean flags ("only", and "ignore"). A trie is the most efficient way to implement that -- no problems there.

However, now we're adding a new feature and saying "now we want each rule to have path-specific configurable variables". To me, that's conceptually the same as what we did with only/ignore, except now instead of boolean flags, we're adding HashMaps and/or string literals. But it's still adding metadata to a path.

I think we even see this repeated abstraction in how you use a trie (appropriately) to implement the arguments.

Did you think through unifying this abstraction? If so, what was the reason not to go forward with it?

[jacobotb]

I don't think they are similar enough to unify them.

Excludes are one-way: you can exclude a file or subtree, but you cannot include a previously-excluded file. Therefore, there are no conflicts: if any 'ignore' or 'only' directive excludes a file, at any level, it is excluded.

The 'only' part of it adds a pretty big wrinkle: it excludes everything except the paths that were specified, but only if the 'only' has been specified too. You could model the 'ignores' as a boolean argument that you can only set to true, but you cannot model 'onlys' as such unless you add code to detect when an 'only' has not been set so that you avoid excluding everything by default.

Arguments may have different values at different levels of the subtree; a file may belong to several of those subtrees at once, which receive different argument values. Therefore, it is necessary to solve the conflict. The intuitive solution is to choose the setting closest to the file -- the tallest subtree prefix. This precludes the use of glob patterns for arguments: there is no concept of "closest" in glob patterns.

Even if they were similar, I have learned to avoid adding abstractions until I have three or more examples. If we keep coming up with more similar features, especially if they bridge the differences between includes/excludes and arguments, we may have a reason to create a unifying abstraction. For now, I think that the differences between them outweigh the similarities.

[jasonforal]

Very helpful explanation -- thanks!