-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Add support for config comments #27
Conversation
src/languages/json-source-code.js
Outdated
} else if (parseResult.ok === false) { | ||
problems.push({ | ||
ruleId: null, | ||
message: parseResult.error.message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I inserted if (parseResult.ok === false)
here because I was getting this TypeScript error without it (with just else
):
error TS2339: Property 'error' does not exist on type '{ ok: true; config: RulesConfig; } | { ok: false; error: { message: string; }; }'.
Property 'error' does not exist on type '{ ok: true; config: RulesConfig; }'.
229 message: parseResult.error.message,
It seems that TypeScript doesn't infer that in else
of if (parseResult.ok)
, parseResult
must be { ok: false; error: { message: string; }; }
. Please let me know if there's a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is happening because we don't use the strictNullChecks
option in tsconfig.json. Without that option, TypeScript assumes that the ok
field in parseResult
could be possibly nullish in both alternatives of the union type, so it cannot infer that the error
property exists from the fact that ok
is falsy.
You could add a JSDoc annotation like /** @type {*} */
above the line where parseResult
is declared to disable type checking in this case, but maybe there's an even better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to do this is with a type guard, something like:
/**
* @param {ParseResult} result
* @returns {result is OkParseResult}
*/
function isOkParseResult(result) {
return result.ok;
}
Then you can use isOkParseResult()
and TypeScript can know that you're positively identifying the type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I fixed this with casting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would like @fasttime to approve before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Prerequisites checklist
What is the purpose of this pull request?
Adds support for rule configuration comments and disable directives.
What changes did you make? (Give an overview)
Added
getInlineConfigNodes()
,getDisableDirectives()
andapplyInlineConfig()
methods to theJSONSourceCode
class.Related Issues
Fixes #21
Is there anything you'd like reviewers to focus on?
Unlike the JS language implementation (SourceCode in eslint/eslint), in this implementation rule configs,
eslint-disable
andeslint-enable
can be Line comments. We wanted to implemented this in the JS language as well (eslint/rfcs#34), and eventually did but reverted the change due to backwards compatibility reasons (eslint/eslint#14960). Since there were no backwards compatibility concerns here, it seemed fine to allow this, but let me know if it would be better to have the same behavior as in the JS language (e.g., ignore comments like// eslint rule
).