Skip to content

Fix lint issues - #4545

Merged
mcmire merged 1 commit into
mainfrom
fix-lint-issues
Jul 22, 2024
Merged

mcmire merged 1 commit into
mainfrom
fix-lint-issues

Conversation

@mcmire

@mcmire mcmire commented Jul 22, 2024 •

Copy link
Copy Markdown
Collaborator

Explanation

For some reason, two lint violations show up on my computer when running yarn lint:eslint. I have attempted to remove node_modules, remove all dist directories, remove .eslintcache, run yarn cache clear, etc. to no avail. I think an ultimate fix here may be to upgrade ESLint packages but that would be a larger change, so as egregious as it is, this commit skirts around the issue by adding some ignore lines.

References

Intermittent lint violations have been a problem for a long time. Also see #4540.

Changelog

(N/A)

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@mcmire
mcmire requested a review from a team July 22, 2024 17:14
For some reason, two lint violations show up on my computer when running
`yarn lint:eslint`. I have attempted to remove `node_modules`, remove
all `dist` directories, remove `.eslintcache`, run `yarn cache clear`,
etc. to no avail. I think an ultimate fix here may be to upgrade ESLint
packages but that would be a larger change, so as egregious as it is,
this commit skirts around the issue by adding some ignore lines.
@mcmire
mcmire force-pushed the fix-lint-issues branch from a874cd6 to a4607bd Compare July 22, 2024 17:21
@Mrtenz

Mrtenz commented Jul 22, 2024

Copy link
Copy Markdown
Member

Did you try restarting your local TypeScript language server?

@mcmire

mcmire commented Jul 22, 2024

Copy link
Copy Markdown
Collaborator Author

@Mrtenz Yeah, I just tried closing my editor, running killall node, explicitly looking for tsserver and killing that, and when I run yarn lint:eslint I'm still getting the two violations.

@mcmire
mcmire merged commit 0f80f16 into main Jul 22, 2024
@mcmire
mcmire deleted the fix-lint-issues branch July 22, 2024 17:26
@Gudahtt

Gudahtt commented Jul 22, 2024

Copy link
Copy Markdown
Member

Not a huge fan of this idea. This type of rule exclusion prevents us from using the reportUnusedDIsableDirectives rule, which would let us treat ESLint ignore comments more like ts-expect-error, in that it would flag cases where they're obsolete.

@mcmire

mcmire commented Jul 22, 2024

Copy link
Copy Markdown
Collaborator Author

@Gudahtt Sure. I wanted to quickly unblock other people if they were encountering this, too, but I admit that this is probably not the best way to deal with this. Is there another way that you would suggest? Are you concerned that we don't currently have a good way to remove these later?

@Gudahtt

Gudahtt commented Jul 22, 2024

Copy link
Copy Markdown
Member

I'd suggest that we investigate this further and find the root cause.

I am not sure what you meant by your last question.

@mcmire

mcmire commented Jul 22, 2024

Copy link
Copy Markdown
Collaborator Author

@Gudahtt You mentioned the use of reportUnusedDisableDirectives. This rule (as I understand it) looks for eslint-disable directives that are unnecessary (because the line that they are referring to no longer has a lint violation). If we were to enable this option and we found that the lint violations I found mysteriously went away, then reportUnusedDisableDirectives should alert us to that so that we can remove those disable directives. Is that true? Or was there some other reason that you mentioned this option?

@Gudahtt

Gudahtt commented Jul 22, 2024

Copy link
Copy Markdown
Member

I mentioned that rule because enabling it would require undoing this change, leaving contributors in the same state of having local lint errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants