-
Notifications
You must be signed in to change notification settings - Fork 36
Enable prefer-array-find unicorn eslint rule #163
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
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
This comment was marked as resolved.
This comment was marked as resolved.
|
@NJ-2020 you need to sign the CLA for this repo before I can merge this. |
|
@roryabraham FYI, I just added new eslint rules for some another tasks. Thanks |
@roryabraham I've already signed the CLA for this repo, but I am still seeing the workflow is failing, is there anything I am missing something? |
|
@NJ-2020 you've got conflicts to fix here. CLA bot is broken by something else. I'll look into it ASAP |
|
I have identified the problem with the CLA. It should be fixed tomorrow |
|
Putting this on HOLD temporarily because publishing is broken in this repo. Hopefully should be fixed tomorrow |
|
CLA is fixed! still working on fixing npm publishing in this repo |
|
npm publish is fixed. Taking this off hold |
Fixed ☑️ Thanks |
|
|
||
| languageOptions: { | ||
| parserOptions: { | ||
| globals: globals.builtin, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@NJ-2020 I think you're having some trouble with merge conficts? Let me help you.
|
And by doing all of these, it will make the commit history clean. Cool, right? |
|
@gelocraft Thank you, FYI I am having some issues with my prettier vscode/codebase |
|
@NJ-2020 I think it would be better to add the lint rules one at a time in separate PRs. Doing them all in a single PR could go against the code of conduct (GET SHIT DONE) and contributing guidelines. |
hmm... not sure about that. i use neovim btw |
|
@roryabraham I think we're good to merge. Tests & lint for the changed files check are passing (tested locally). |
@roryabraham wouldn’t submitting fixes for all assigned issues in a single PR go against the project’s contributing guidelines and code of conduct? Each issue should ideally have its own PR, right? |
@gelocraft FYI, usually I always do separate PR for each issue (E/App, expensify-common), but at this point (eslint-config-expensify), I don't think it's really necessary to create separate PR for each new rule (small changes) and the current PR doesn't need too much review and since this is straightforward PR and hasn't been merged yet, so I don't see any issues including other rule. If this is still against (eslint-config-expensify) the project’s contributing guidelines and code of conduct, I will make sure to create separate PR for each lint/rule/issue Thanks. |
I certainly don't think it's a code of conduct violation. It may be somewhere in the contributing guidelines that smaller PRs are better, because that's certainly one of our best practices. But in some cases, the same change might naturally fix multiple bugs, and that's ok. In this case, I agree with @gelocraft that it would be better to add each rule in a separate pull request. That way we get more granular releases, allowing for separate E/App PRs to enable each rule individually. That is in everyone's best interest for several reasons:
|
roryabraham
left a comment
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.
Let's enable all the rules in separate PRs
|
@roryabraham Done, updated ☑️ |
|
Draft PRs is here (HOLD): |
|
@roryabraham I think we're good to merge 😁, please let me know if you need anything from my side. Thanks |
Explanation of Change
Enforce that
.findis being used instead of.filterwhen matching one/1 element, and enforce to use.findLastwhen trying to match the last element$: Expensify/App#67423
PROPOSAL: Expensify/App#67423 (comment)