-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
chore: lint and typecheck top level #8880
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR, @abrahamguo! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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'm not a fan of adding in "secondary" code paths, like having two ways of running linting or type checking &&
d together. It'd be less code & complexity to get the "primary" (npx nx ...
) commands working as expected.
Let's fine a way to include what's under .github/
and tools/
under nx. packages/repo-tools
now exists, maybe that's a good place for some files?
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.
Thanks for looking into this!
It turns out that |
@JoshuaKGoldberg I've moved all scripts from However, let me know your thoughts about the remaining files which are still at the top level:
|
@@ -40,21 +40,19 @@ | |||
"lint-markdown-fix": "yarn lint-markdown --fix", | |||
"lint-markdown": "markdownlint \"**/*.md\" --config=.markdownlint.json --ignore-path=.markdownlintignore", | |||
"lint-stylelint": "npx nx lint website stylelint", | |||
"lint": "npx nx lint eslint-plugin --skip-nx-cache && npx nx run-many --target=lint --parallel --exclude eslint-plugin", | |||
"lint": "npx nx lint eslint-plugin --skip-nx-cache && npx nx run-many --target=lint --parallel --exclude eslint-plugin && eslint . --ignore-pattern=packages --cache", |
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.
This still does the &&
strategy 😕. Surely there's got to be a way in Nx to set up a project to include root-level files, so the npx nx run-many --target=lint
run would include them?
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.
Sure thing, I can look into that
I can provide feedback tomorrow, in all day planning meetings today |
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.
Just adding a blocker until I can look tomorrow
👋 @JamesHenry is this still something you want to look at? |
@abrahamguo I have just updated the repo to use nx latest (19.4.0) and am pulling down your branch now to look at it. I will update the merge conflicts |
👋 @abrahamguo just checking in, are you still waiting on us for anything? We're hoping to close out v8 tasks soon, so we're going to start applying more time pressure on this and other PRs that address issues in the v8 milestone. Just a heads up. 🙂 |
I'm not very familiar with |
@JoshuaKGoldberg this is my fault I didn’t get to push up my updates before my vacation without my laptop. I return on Friday evening so will get to it this weekend or latest beginning of next week. thank you @abrahamguo for your patience and understanding |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8880 +/- ##
=======================================
Coverage 88.45% 88.45%
=======================================
Files 422 422
Lines 14694 14694
Branches 4299 4299
=======================================
Hits 12997 12997
Misses 1372 1372
Partials 325 325
Flags with carried forward coverage won't be shown. Click here to find out more. |
Heh, the chain is a bit convoluted. It's blocking #8196, which we're considering something we should do internally as a form of validation before launching v8 as stable. |
PR Checklist
packages
are not being linted or typechecked #8879Overview
Ensure all files outside of
packages
are linted and typechecked.