-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
🏗✨ Adopt prettier
for code formatting
#21212
Conversation
The most egregious to me: prettier/prettier#5588 |
This is super exciting! Thanks for documenting all the issues you're facing during the migration. As a quick workaround of the html template issue, I believe that you could call .trim() on the string and that would fix the issues. |
I'm curious, what tooling is using the |
Now, we use eslint and check the |
We have a new Prettier release with bug fixes at https://prettier.io/blog/2019/04/12/1.17.0.html. (Yay!) I synced to Current status:
There may be ways to work around 2 and 4, but I think 3 is still a dealbreaker. /to @cramforce @jridgewell @alanorozco @aghassemi for comment |
I ran some checks on the new code.
|
4 need not be fixed for us to adopt prettier, our code will work just fine. My dislike for it is only because it is an actual code change, not a formatting change. I think only 3 needs to be fixed before we can merge this. |
Update: We have a new Prettier release 1.17.1. With this, none of the issues mentioned in #21212 (comment) are blockers any more. This PR has been updated with the latest version, and is now ready for review. See PR description for details on what has changed. |
This PR is now ready for a full review. Some notes:
Tagging a few reviewers to distribute the load. Please post a comment if you see any objectionable changes in the code you own: @cramforce @aghassemi @choumx @newmuis @lannka @Gregable @jpettitt @kristoferbaxter @rsimha Once this is approved for merge, I will rebase the entire PR on the latest |
/cc @gmajoulet @Enriqe can you take a look for stories while I'm out? |
@newmuis sure! |
This PR adopts Prettier as the de-facto formatting standard for the
ampproject/amphtml
codebase.Changes, in order:
prettier
as aneslint
plugin.eslintrc
withprettier
/*OK*/
annotations associated with function calls to prefix the function name@type
annotations in AMP codevalidator/
from linting, since it uses a separate set of rules internal to Googlegulp lint --fix
no-useless-concat
lint errors that arose as a result of prettier's fixeseslint-disable-line
annotations toeslint-disable-next-line
New developer workflow:
gulp lint --local-changes --fix
before uploading a commit.Rebasing your working branch:
To rebase an in-flight PR that has merge conflicts with
master
due to this PR, use the steps outlined here.Fixes #17086
Closes #18601
Closes #13966