Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Warn for deprecation in Node output (fixes #7443) #10953

Merged
merged 8 commits into from Oct 30, 2018

Conversation

@calling
Copy link
Contributor

commented Oct 10, 2018

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

#7443

What changes did you make? (Give an overview)
Added a property usedDeprecatedRules to the output of the Node API that flags usage of deprecated rules.

Is there anything you'd like reviewers to focus on?
The issue touched on other possible work (i.e. formatter changes, usedRemovedRules, usedNotFoundRules). From reading the issue, I think the group settled on only implementing usedDeprecatedRules, but please check my understanding.

@eslint eslint bot added the triage label Oct 10, 2018

@calling calling force-pushed the calling:issue/7443 branch 2 times, most recently from 8f95777 to f30fb69 Oct 10, 2018

@calling calling changed the title Fix: add deprecation warning to Node API output (#7443) Fix: Warn for deprecation in Node output ( (#7443) Oct 10, 2018

@calling calling changed the title Fix: Warn for deprecation in Node output ( (#7443) Fix: Warn for deprecation in Node output (#7443) Oct 10, 2018

@calling calling force-pushed the calling:issue/7443 branch from f30fb69 to 3c049d0 Oct 10, 2018

@calling calling changed the title Fix: Warn for deprecation in Node output (#7443) Fix: Warn for deprecation in Node output (fixes #7443) Oct 10, 2018

@kaicataldo
Copy link
Member

left a comment

Thanks for contributing!

The code here LGTM, but I wanted to ask about the additional properties mentioned in the implementation described in this comment (usedRemovedRules and usedNotFoundRules). Did we decide not to implement those? Regardless, I'd be fine with merging this as is and adding the other two properties in a follow up PR, if we decide those would still be nice to have.

function createRuleWarnings(rules, loadedRules) {
const ruleWarnings = { usedDeprecatedRules: [] };

if (!rules) {

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Oct 19, 2018

Member

I know this isn't a terribly complex function, but I wonder if we could reduce the complexity a bit by restructuring it as:

const ruleWarnings = { usedDeprecatedRules: [] };

if (rules) {
    Object.keys(rules).forEach(name => {
        const loadedRule = loadedRules.get(name);

         if (loadedRule.meta && loadedRule.meta.deprecated) {
            ruleWarnings.usedDeprecatedRules.push({ ruleId: name, replacedBy: loadedRule.meta.docs.replacedBy });
        }
    });
}

return ruleWarnings;

This comment has been minimized.

Copy link
@calling

calling Oct 19, 2018

Author Contributor

No problem!

@platinumazure
Copy link
Member

left a comment

LGTM, thanks for contributing!

@calling

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2018

@kaicataldo I'm also unsure the outcome of the discussion w.r.t. those other two properties (usedRemovedRules and usedNotFoundRules). Perhaps we can address that as a follow up?

@not-an-aardvark
Copy link
Member

left a comment

Thanks for the PR! I left a few minor suggestions, but overall this approach seems good to me.

const loadedRule = loadedRules.get(name);

if (loadedRule.meta && loadedRule.meta.deprecated) {
ruleWarnings.usedDeprecatedRules.push({ ruleId: name, replacedBy: loadedRule.meta.docs.replacedBy });

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Oct 19, 2018

Member

Will this work correctly if a rule doesn't have a meta.docs property (e.g. a rule from a plugin)?

More generally, I'm wondering if we should move meta.docs.replacedBy to just meta.replacedBy, since the replacedBy field doesn't really relate to documentation in this context. (I know it's currently meta.docs.replacedBy on existing core rules, but it's easier to change that now than later since it's not a public API yet.)

edit: In either case, I think we should also update the Working with Rules page to note the new available options in the rule API.

This comment has been minimized.

Copy link
@calling

calling Oct 19, 2018

Author Contributor

That's a great point about meta.docs not necessarily existing. The "Working with Rules" page explicitly says that meta.docs is optional for custom rules and plugins: "In a custom rule or plugin, you can omit docs or include any properties that you need in it".

I'm happy to move replacedBy up to meta or I can add a property check (and document it either way), but I think where replacedBy should hang is best decided by the maintainers?

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Oct 23, 2018

Member

My personal preference would be to move it to meta.replacedBy rather than meta.docs.replacedBy, but let's see what other team members think.

cc @kaicataldo @platinumazure

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Oct 28, 2018

Member

Moving replacedBy to meta.replacedBy makes sense to me 👍

@@ -516,7 +519,7 @@ The top-level report object has a `results` array containing all linting results
* `source` - The source code for the given file. This property is omitted if this file has no errors/warnings or if the `output` property is present.
* `output` - The source code for the given file with as many fixes applied as possible, so you can use that to rewrite the files if necessary. This property is omitted if no fix is available.

The top-level report object also has `errorCount` and `warningCount` which give the exact number of errors and warnings respectively on all the files.
The top-level report object also has `errorCount` and `warningCount` which give the exact number of errors and warnings respectively on all the files. Additionally, `usedDeprecatedRules` signals any deprecated rules used and their replacement (if available).

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Oct 19, 2018

Member

Could you be more specific in this documentation about what the shape of an object in usedDeprecatedRules would look like (namely, that it has a ruleId and replacedBy property)?

const loadedRule = loadedRules.get(name);

if (loadedRule.meta && loadedRule.meta.deprecated) {
ruleWarnings.usedDeprecatedRules.push({ ruleId: name, replacedBy: loadedRule.meta.docs.replacedBy });

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Oct 19, 2018

Member

I think we should only include the replacedBy key if it's a string, and omit it otherwise. If a rule from a plugin does something weird and uses a different type of object as a replacedBy value, it could have the effect of crashing an integration/formatter.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

Regarding usedRemovedRules, I think it's probably not worth adding special logic for removed rules because we no longer remove any rules and it's been a long time since we removed any rules, so it seems unlikely that many users are still enabling removed rules. At some point it might be a good idea for us to get rid of the remaining "removed rule" logic and just treat removed rules the same way as nonexistent rules, although that doesn't need to be decided here.

@nzakas

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

@not-an-aardvark it looks like your changes may have been addressed. Can you take another look?

@platinumazure

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

I agree it would be nice to move to meta.replacedBy, and agree it should be done sooner than later if we're going to do it; but this is a slight preference, not a strong preference.

@calling

This comment has been minimized.

Copy link
Contributor Author

commented Oct 28, 2018

Seems like the enough folks want meta.docs.replacedBy moved to meta.replacedBy, so I've moved it.

@kaicataldo
Copy link
Member

left a comment

LGTM. Thanks for contributing!

@nzakas

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

@not-an-aardvark looks like we just need an approval from you to move this forward. Can you take another look?

@not-an-aardvark
Copy link
Member

left a comment

LGTM, thanks!

@nzakas nzakas merged commit 802e926 into eslint:master Oct 30, 2018

5 checks passed

commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@nzakas

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

Merged, thanks for the contribution. This will be super helpful to users.

@calling calling deleted the calling:issue/7443 branch Oct 30, 2018

@eslint eslint bot locked and limited conversation to collaborators Apr 29, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.