fix: unify LintMessage type
#17076
Merged
fix: unify LintMessage type
#17076
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
btmills
added
the
evaluating
btmills
force-pushed
the
chore-unify-lint-message-type
branch
3 times, most recently
from
d2604b7
to
406c2b0
Compare
nzakas
approved these changes
Apr 12, 2023
There was a problem hiding this comment.
LGTM, thanks for doing this, and thanks for the tip about reviewing commit-by-commit.
Just want to leave open for @mdjermanovic to review. (He's on holiday this week, so will likely not be until next week.)
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
|
Don't close. Not stale. @btmills there are a few review comments for you to look into. Thanks. |
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
|
I believe this is still not stale. cc @btmills |
I made three changes to `LintMessage` and `SuppressedLintMessage`:
1. Many places that create `LintMessage`s don't set `fatal`, so I made
it optional. This would be a breaking change if we made types part of
our exports, but in this case I believe it's updating JSDoc
documentation to reflect reality.
2. Added an optional `messageId` property that's set by reports from rules.
3. Added an optional, nullable `nodeType` property.
- Reports from rules set it to a value.
- `applyDirectives()` explicitly sets it to `null` for unused
disable directives.
- `Linter`'s `createLintingProblem()` explicitly sets it to `null`.
Two open questions:
1. Should I make `nodeType` non-nullable and update the places in the
implementation that explicitly set it to `null` to omit it instead? A
few places already omit it for e.g. parse errors or ignored file
messages, so API consumers already have to check that it might not be
set.
2. `line` and column` are currently required but possibly `undefined`.
- Ignored file messages currently omit `line` and `column`.
- `Linter` explicitly sets them to `0` for two configuration errors
(configured parser not found and no configuration found for file).
- `Linter`'s `createLintingProblem()` has a `DEFAULT_ERROR_LOC` that
defaults to line 1 column 0, but the default is only used for
missing rule definition messages.
Should I either:
a. Keep `line` and `column` required but possibly `undefined` and
update ignored file messages to explicitly set them to `0`,
following precedent elsewhere?
b. Or make `line` and `column` optional and remove fallback 0/0 or
1/0 values from configuration issues?
Option b is how I'd design the API from scratch because I wouldn't
report a location unless we had a real locationl. But it might be a
breaking change. Even though the type is specified as possibly
`undefined`, in practice, some API consumers might only see messages
that have real or fallback locations if they don't encounter ignored
file messages. Option b would slightly increase the prevalence of
location-less messages.
All of these previously referenced a `Problem` type that does not have a definition.
`ReportInfo` was defined within `report-translator.js` to be very similar to `LintMessage`. It had one extra property, `source`, which wasn't ever set, so it would've been removed anyway. In `Linter.runRules()`, the return value from `reportTranslator()` gets pushed into a `lintingProblems` array and returned, and the return type annotation is `LintMessage[]`, which gives me confidence that `ReportInfo` was intended to be the same type as `LintMessage` elsewhere.
Originally, we talked about the reverse - making `ruleId` optional but non-nullable and relying on `report-translator.js`'s `createProblem()` to normalize it. However, the `LintMessage` type would differ from `createProblem()`'s return value only by `null` vs `undefined` for `ruleId`. So instead, I made `ruleId` required but nullable and explicitly set it to `null` in the few remaining places that didn't already.
`LintMessage.nodeType` is currently defined as required but nullable. Actual implementations explicitly set it to `null` in a couple places and omit it in several. After discussion in eslint#16968, we initially leaned toward making it non-nullable but optional. I pursued that, but it resulted in slightly more runtime code changes, including some branching in `report-translator` to set it conditionally. Instead, I'm presenting the opposite solution of updating the remaining implementations to match the existing type definition by explicitly setting `nodeType` to `null`.
This updates existing documented properties to match the updated `LintMessage` type and implementations. `messageId`, `nodeType`, and `suppressions` remain undocumented.
btmills
force-pushed
the
chore-unify-lint-message-type
branch
from
c519894
to
55bed08
Compare
|
@mdjermanovic can you take another look? |
mdjermanovic
changed the title
LintMessage typeLintMessage type
mdjermanovic
removed
evaluating
mdjermanovic
added
core
|
I changed this to |
1 task
|
Re-submitted in #17265 |
eslint-github-bot
bot
added
the
archived due to age
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: Update JSDoc types
What changes did you make? (Give an overview)
Fix #16968, changing runtime code as little as possible. First, this updates the
LintMessagetype to match the most common implementation. It also combines some undefined types or duplicate definitions intoLintMessageso everything references the single definition. Finally, it normalizes the few remaining divergent implementations in three ways: 1) always set alineandcolumn, even if they're 1/0, 2) always setruleId, even if it'snull, and 3) always setnodeType, even if it'snull. Were we designing the API from scratch, we might make those properties optional, but for this existing API, I followed the most common precedent of providing dummy values.Is there anything you'd like reviewers to focus on?
This will be easiest to review going one commit at a time, and see each commit message for more context.
For
nodeType, I changed direction from where we were leaning in #16968 after the original direction became a bit more complicated. There are three possible solutions:nodeTypenon-nullable but optional. This is where we were originally leaning. However, it had the most significant runtime code change, adding branching inreport-translatorto set it conditionally.nodeTypenullable and required. This is what's currently in this PR. This way, all three changes do conceptually the same thing of providing an "emtpy" value for a required property. That felt cleaner than makingline/column/ruleIdrequired yetnodeTypeoptional. It's larger in terms of absolute lines in the diff because I had to add morenulls, but that's all I had to do.nodeTypenullable and optional. This would only require changing the type definition. After seeing thenulls added in thenodeTypecommit, let me know if you'd like me to go with this option instead. It's not consistent with the others, but it does match current reality.