-
Notifications
You must be signed in to change notification settings - Fork 2
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
comment all errors, prevent duplicate rules #10
base: master
Are you sure you want to change the base?
Conversation
Hi! I appreciate the PR. This seems like a good thing to get in. Would you mind resubmitting without your prettier changes? I had the code formatted the way I liked it, and also it makes the diff very hard to read. 😄 |
Hey @NickHeiner ! I'm hoping to get around to it soon. Do you have a prettier config file you could check into the repo to make it easier to reformat and work on the repo without making major modifications? |
I don't use prettier for this project – formatting is handled by ESLint. I'm guessing you have a "format on save" feature enabled on your editor – I'd just disable that. Then run |
return `// eslint-disable-next-line ${rules.join(' ')}`; | ||
function getEslintDisableComment(rules) { | ||
const ruleSet = new Set(rules); | ||
return `/* eslint-disable-next-line ${Array.from(ruleSet).join(', ')} */`; |
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.
using /* ... */
style comments made it easier to use regex to look for the new comments and fix broken comments in jsx
.options({ | ||
rule: { | ||
alias: 'r', | ||
array: true, | ||
string: true, | ||
demandOption: true, |
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.
removed demandOption
from rule, if rule is not provided - run this for any rule
hey @NickHeiner ! I finally got around to patching this fork, I added my reasoning for a few changes in separate comments. This PR should be a lot more review-able now 😛 |
poke 👉 @NickHeiner |
Thanks! I may not have time to review this for a few weeks, though.
…On Fri, May 27, 2022 at 16:33 Baruch-Adi Hen ***@***.***> wrote:
poke 👉 @NickHeiner <https://github.com/NickHeiner>
—
Reply to this email directly, view it on GitHub
<#10 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGKTAYZ2IE2D66GPL2GMG3VMDMNLANCNFSM5UST5MHQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This PR still has a ton of diff noise. 😄 Can you remove all extraneous formatting changes? |
I pulled apart the changes in the PR here to separate them from the formatting changes. From this, I made a commit that tests some of those changes. I'm not sure it's completely ready for a PR, since I didn't include this portion of the original PR. @NickHeiner would you be able to drive this the rest of the way to completion? |
What this PR addresses:
declare-eslint-bankruptcy ./my-dir --explanation "TODO: Fix ESLint Error (#12345)"
NOTE: my prettier went to town on these files, sorry about that!