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

restore types linting #8058

Merged
merged 10 commits into from
Jul 17, 2023
Merged

restore types linting #8058

merged 10 commits into from
Jul 17, 2023

Conversation

turadg
Copy link
Member

@turadg turadg commented Jul 17, 2023

closes: #5788
refs: #7888

Description

Types linting got very slow so we had to disable it.

typescript-eslint/typescript-eslint#6754 solved a problem in linting plugin whereby it was repeating work.

It worked by operating more like the IDE so I thought it would also help with #4620 , but when I tested in VS Code the ESLint plugin failed. I bet it will get worked out eventually. Meanwhile we can still use this new option for CI runs, restoring our floating-promise warnings and opening the door to error on them.

Security Considerations

Scaling Considerations

Documentation Considerations

Reviewers, it is recommended to review by commit.

Testing Considerations

CI

The two lint jobs each take 5min. (Only 30s of that is setup overhead so they could be combined into one job that takes ~10min, which is still faster than most of the jobs. It might be even faster that than since this new option re-uses projects. But I'll leave that consolidation to another PR since it affects the GH repo "required checks" rules.)

@@ -1,4 +1,5 @@
#!/usr/bin/env node
// @ts-nocheck
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was dismayed by all these @ts-nocheck until I realized that they weren’t previously checked and that you’ve just moved the exclusion from jsconfig.json to individual files, presumably so we can reveal them to the checker individually. Carry on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bingo. glad that was apparent enough since I was too lazy to leave a comment explaining

@@ -36,6 +36,8 @@ module.exports = {
parser: '@typescript-eslint/parser',
parserOptions: lintTypes
? {
// this is not yet compatible with eslint lsp so it's conditioned on AGORIC_ESLINT_TYPES
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend “gated”, “contingent on”, or “predicated”.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all better, though imo not different enough to suffer through the Chain deployment test again.

@turadg turadg added this pull request to the merge queue Jul 17, 2023
Merged via the queue into master with commit 6f740a4 Jul 17, 2023
59 checks passed
@turadg turadg deleted the 4620-ts-project branch July 17, 2023 22:57
@turadg turadg mentioned this pull request Jul 18, 2023
mhofman pushed a commit that referenced this pull request Aug 7, 2023
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.

linting types disabled (too slow)
2 participants