-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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: eslint-plugin-node to eslint-plugin-n #26907
feat: eslint-plugin-node to eslint-plugin-n #26907
Conversation
Please provide some links/references |
yes, it's just a wip. will provide more info and sign the cla later. |
I guess one challenge here is that eg the eslint rules are renamed from Can this be handled? |
no |
Then sadly I think this isn't possible to do right now. Would be nice if one could eg. show a replacement suggestion in the Dashboard issue, like:
|
renovate should simply open the replacement PR and let a maintainer fix the migration needed. |
91dbe17
to
4c69287
Compare
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.
The name got a bit mixed up with npm-run-all
😜
@viceice That assumes that people's tests will fail so that they realize that they need to do additional manual changes To make matters even worse: If they are using a shared ESLint config, then it might not even be they who should make the change, it should be the upstream |
860508e
to
7efcf5a
Compare
updated the OP, and signed the CLA.🎉 |
Can you give an example of a situation where a ci won't fail?🤔 |
When there is no CI :) Not everyone runs their ESLint on CI |
that's not renovate's fault. renovate won't automerge r placements by default, so it's the developer who is responsible to check required changes before merging such PRs. |
Lets try it then and see what the feedback is, would be great to get people moving to it 👍 |
ef375dc
to
640ca4f
Compare
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.
please don't force-push in future, we're doing squash merge
@aladdin-add Can you maybe update the title of this PR to refer to |
my bad! 😅 updated~ |
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.
Small style change to the preset name maybe?
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 approval is only for the text content. 😉
I'll leave it to the maintainers to approve this preset, and to review if this fork is the best replacement.
Some added context here that may be helpful for them: This is the fork that's maintained by the official ESLint Community – a home for high-value projects in the ESLint ecosystem where the ESLint community collectively can ensure that they continue to be maintained instead of eg. relying on any single maintainer / releaser. |
friendly ping... |
So I think we confirmed:
But is there a problem if this PR breaks people's scripts due to migration being necessary? |
I think we should limit current version to |
still conflicted |
e4da263
to
9c1bfc7
Compare
should be addressed now. |
🎉 This PR is included in version 37.256.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
it was forked from eslint-plugin-node v11.1.0. as the original repository seems no longer maintained.
repo link: github.com/eslint-community/eslint-plugin-n
npm link: https://www.npmjs.com/package/eslint-plugin-n
related discussion: mysticatea/eslint-plugin-node#300
Changes
Context
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via: