Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Better handling of invalid rule configuration options #4280

Closed
JoshuaKGoldberg opened this issue Nov 8, 2018 · 5 comments
Closed

Better handling of invalid rule configuration options #4280

JoshuaKGoldberg opened this issue Nov 8, 2018 · 5 comments

Comments

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Nov 8, 2018

Continuing discussion in #4206: how should rules display that their configurations are invalid?

Right now, as @cheeZery suggested, they can use the showWarningOnce strategy and not run. Should there be a standard utility for this?

More grandly, what if there were a system to validate options once per rule on startup? Rules already have JSON schemas specified, can we use it to remove the burden of validity checking from each rule?

@cheeZery
Copy link
Contributor

cheeZery commented Dec 9, 2018

I did some trial by error for this validation feature. I found that the method to validate the options itself wouldn't be that hard to implement. For example jsonschema seems suited for the job
and the parseConfigFile method in src\configuration.ts might be a good place to implement that logic. Something like:

    function validateRulesOptions(
        rules: Map<string, Partial<IOptions>>,
        rulesDirectory?: string | string[],
    ): Map<string, Partial<IOptions>> {

        const validator = new Validator(); // import { Validator } from "jsonschema";
        rules.forEach((ruleOptions, ruleName) => {
            // TODO: Make sure ruleSeverity is not "off" and ruleArguments aren't undefined

            const Rule = findRule(ruleName, rulesDirectory);
            // TODO: Make sure Rule is defined and has metadata

            if (Rule.metadata.options === null) {
                if (ruleOptions.ruleArguments.length !== 0) {
                    // TODO: Print warning that given rule doesn't expect any options
                }
                return;
            }

            const validationResult = validator.validate(ruleOptions.ruleArguments, Rule.metadata.options);
            if (!validationResult.valid) {
                // TODO: Print warning that given options don't match rule's JSON schema, maybe also print validation errors
            }
        });
    }

The tricky part is how the JSON schema for the options is currently written and how the options are written in tslint.json. The rule array-type for example has on string option. The JSON schema is declared as:

        options: {
            type: "string",
            enum: [OPTION_ARRAY, OPTION_GENERIC, OPTION_ARRAY_SIMPLE],
        },

But since ruleArguments are always arrays, the option will be something like ["array-simple"], so the validation would fail. But we obviously also can't just parse the first item of the array to the validation, because there are rules, where more than one string can be provided, e.g. comment-type rule (there the option is declared as array of strings, so it works just fine). Optional options are a whole nother problem.
Long story short: I guess either a lot of JSON schemata had to be fixed or the way options are declared (e.g. for more than one string [true, ["foo", "bar"]] instead of [true, "foo", "bar"].
Maybe the problem can be solved with the new way of declaring rule options with an option prop.

@rafaelss95
Copy link
Contributor

@cheeZery do you pretend to submit a PR with this?

@cheeZery
Copy link
Contributor

cheeZery commented Apr 4, 2019

@rafaelss95 actually no, I'm not :) since this issue still got the tags "In Discussion" and "Needs Approval".
In light of #4534 I'm also not sure the guys are willing to allow/implement such a change which has potential to break some stuff (e.g. if users and/or plugins misconfigured their rules).
Although it also might be easier to migrate to a new system when solid tests are in use.

@JoshuaKGoldberg
Copy link
Contributor Author

💀 It's time! 💀

TSLint is being deprecated and no longer accepting pull requests for major new changes or features. See #4534. 😱

If you'd like to see this change implemented, you have two choices:

  • Recommended: Check if this is available in ESLint + typescript-eslint
  • Not Recommended: Fork TSLint locally 🤷‍♂️

👋 It was a pleasure open sourcing with you!

If you believe this message was posted here in error, please comment so we can re-open the issue!

@JoshuaKGoldberg
Copy link
Contributor Author

🤖 Beep boop! 👉 TSLint is deprecated 👈 (#4534) and you should switch to typescript-eslint! 🤖

🔒 This issue is being locked to prevent further unnecessary discussions. Thank you! 👋

@palantir palantir locked and limited conversation to collaborators Mar 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants