-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
fix: Eslint on Danger not matching project config #34980
Conversation
Base commit: a575472 |
Base commit: a575472 |
// Ensures that eslint is run from root folder and that it can find .eslintrc | ||
process.chdir('../../'); | ||
eslint.default(); |
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.
Would it also work if you instead set options.cwd
?
Documentation: https://eslint.org/docs/latest/developer-guide/nodejs-api
// Ensures that eslint is run from root folder and that it can find .eslintrc | |
process.chdir('../../'); | |
eslint.default(); | |
eslint.default({ | |
// Ensures that eslint is run from root folder and that it can find .eslintrc | |
cwd: '../../', | |
}); |
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.
Not really, that was my first approach too @yungsters but unfortunately @sgtcoolguy/danger-plugin-eslint
only allows us to change the baseConfig
.
https://github.com/sgtcoolguy/danger-plugin-eslint/blob/master/src/index.ts#L24
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.
Oh no… thanks for sharing. And yes, as @cipolleschi shared, thanks for fixing! I suspect this was one of the issues that blocked me from making progress on #34401. 🥰
Woah, Great finding @gabrieldonadel! I think this happened when we moved danger in a node package within the |
@yungsters has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This pull request was successfully merged by @gabrieldonadel in 55b09da. When will my fix make it into a release? | Upcoming Releases |
Summary: I've been noticing this for a while now, every time that someone changes a file inside rn-tester Danger comments a bunch of warnings regarding inline styles, e.g. facebook#34567 (comment), even though that is disabled inside .eslintrc https://github.com/facebook/react-native/blob/8edf4e9e3adcc85743e3c86a25ab3748b276a3da/packages/rn-tester/.eslintrc#L3 After some investigation, I realized that the problem was that the Eslint Node.js API used by [seadub/danger-plugin-eslint](https://www.npmjs.com/package/seadub/danger-plugin-eslint) was not able to locate any `.eslintrc` files due to the location of the directory where danger is invoked. By using `process.chdir` we can ensure that eslint will run from the root folder and that it will be able to find all .eslintrc files ## Changelog [Internal] [Fixed] - fix eslint config when running Danger Pull Request resolved: facebook#34980 Test Plan: run `yarn danger pr https://github.com/facebook/react-native/pull/34976` After ![image](https://user-images.githubusercontent.com/11707729/195751496-5225a32f-f4b3-4ce2-833f-ae5723f647c0.png) Before ![image](https://user-images.githubusercontent.com/11707729/195751673-34ba87fc-ce50-4020-9688-a486e3021c4f.png) Reviewed By: cortinico Differential Revision: D40384300 Pulled By: yungsters fbshipit-source-id: e68eeafc42567dc9d7297dde3709a989cc70f4e2
Summary: I've been noticing this for a while now, every time that someone changes a file inside rn-tester Danger comments a bunch of warnings regarding inline styles, e.g. facebook#34567 (comment), even though that is disabled inside .eslintrc https://github.com/facebook/react-native/blob/8edf4e9e3adcc85743e3c86a25ab3748b276a3da/packages/rn-tester/.eslintrc#L3 After some investigation, I realized that the problem was that the Eslint Node.js API used by [seadub/danger-plugin-eslint](https://www.npmjs.com/package/seadub/danger-plugin-eslint) was not able to locate any `.eslintrc` files due to the location of the directory where danger is invoked. By using `process.chdir` we can ensure that eslint will run from the root folder and that it will be able to find all .eslintrc files ## Changelog [Internal] [Fixed] - fix eslint config when running Danger Pull Request resolved: facebook#34980 Test Plan: run `yarn danger pr https://github.com/facebook/react-native/pull/34976` After ![image](https://user-images.githubusercontent.com/11707729/195751496-5225a32f-f4b3-4ce2-833f-ae5723f647c0.png) Before ![image](https://user-images.githubusercontent.com/11707729/195751673-34ba87fc-ce50-4020-9688-a486e3021c4f.png) Reviewed By: cortinico Differential Revision: D40384300 Pulled By: yungsters fbshipit-source-id: e68eeafc42567dc9d7297dde3709a989cc70f4e2
Summary
I've been noticing this for a while now, every time that someone changes a file inside rn-tester Danger comments a bunch of warnings regarding inline styles, e.g. #34567 (comment), even though that is disabled inside .eslintrc
react-native/packages/rn-tester/.eslintrc
Line 3 in 8edf4e9
After some investigation, I realized that the problem was that the Eslint Node.js API used by @seadub/danger-plugin-eslint was not able to locate any
.eslintrc
files due to the location of the directory where danger is invoked. By usingprocess.chdir
we can ensure that eslint will run from the root folder and that it will be able to find all .eslintrc filesChangelog
[Internal] [Fixed] - fix eslint config when running Danger
Test Plan
run
yarn danger pr https://github.com/facebook/react-native/pull/34976
After
Before