-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Transform arrow functions #1281
Transform arrow functions #1281
Conversation
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 think we should enable arrow-body-style as "as-needed" in this PR, and also arrow-parens as "as-needed".
The option for each rule, we can leave off for now, but those changes will be smaller, and I'd prefer to keep the changes to all the arrow fns in the same PR.
return body.filter(function(item) { | ||
return item.type === 'ReturnStatement'; | ||
})[0]; | ||
return body.filter(item => item.type === 'ReturnStatement')[0]; |
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.
Leaving a reminder to myself later to change this to .find
methodVer = methodVer.split('.').map(function(part) { | ||
return Number(part); | ||
}); | ||
methodVer = methodVer.split('.').map(part => Number(part)); |
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 could really just be .map(Number)
I've added the 2 remaining rules without the options. I'm sorry, I might be misunderstanding you. Should I add those options (ex.: Also does this mean that shorthand-methods should also come in this PR? (I'll merge from master tomorrow morning to resolve conflicts) |
Thanks! Let's not add As for (Please rebase from the command line, not merge, so as to avoid merge commits) |
Yes, all autofixes, wanted to add manual fixes in a separate commit but wasn't needed in this case AFAIK. I'll look into how to rebase than, so far mostly used the Desktop app. |
I failed. Here's what I did: Updated my master (dfilipidisz/eslint-plugin-react) from origin master (yannickcr/eslint-plugin-react) based on this. Started rebase on the PR branch:
Didn't sqash or change commit messages. (Although I think I should have changed one to be more descriptive) There was a conflict in the void-dom-elements-no-children rule.
was conflicting with this part.
And I didn't realize that those parts were in different parts of the file, and were conflicted because the line numbers changed because of my previous commits (I assume).
And than the rebase finished. Now the real problem was that I forgot to run My question is how to proceed?
I have a backup branch from before the rebase, so I think with these steps I could fix what I've done, on the cost of another commit. |
You definitely don't need to ever scrap a PR! Hm, If it's too much trouble I'm happy to rebase it for you; extra commits and frequent force pushes are fine tho :-) |
Oh okay than, if I'm not causing any major trouble I would like to try to fix this mess I made. It's definitely a learning experience for me, but it's good to know someone can help, thanks! |
Take your time; I'm happy to step in when needed. Always better to fix an existing PR than to create clutter by opening up a new one :-) Just make sure not to delete your remote branch, or the PR will be unrecoverable! |
At this point I believe I'm just making matters worse. The master branch passes 2373 test and this PR passes 2363 tests when running I've added back the code part I know I erased while resolving badly a conflict during the first rebase. |
@dfilipidisz certainly this needs a rebase since it contains merge commits - if it's become untenable to do that, perhaps we reset the branch to master, rerun the codemods, and force push up the results? |
@ljharb Okay. So before I mess up again:
|
@dfilipidisz yes, all your steps look good. You can use |
95119c9
to
a8cd2fe
Compare
Re-opening. Re-run everything, test pass, nothing seems to be messed up so far. |
Used the following command to do the transformation:
I left out the following eslint rules because I don't know enough about the style direction of the project: