Skip to content
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

Merged
merged 21 commits into from
May 16, 2019
Merged

🏗✨ Adopt prettier for code formatting #21212

merged 21 commits into from
May 16, 2019

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Mar 1, 2019

This PR adopts Prettier as the de-facto formatting standard for the ampproject/amphtml codebase.

Changes, in order:

  • Installed the latest version of prettier as an eslint plugin
  • Replaced all formatting rules in .eslintrc with prettier
  • Moved all /*OK*/ annotations associated with function calls to prefix the function name
  • Fixed several incorrect @type annotations in AMP code
  • Exempted validator/ from linting, since it uses a separate set of rules internal to Google
  • Ran Prettier against the entire codebase using gulp lint --fix
  • Fixed all no-useless-concat lint errors that arose as a result of prettier's fixes
  • Switched some eslint-disable-line annotations to eslint-disable-next-line
  • Added documentation with editor recommendations and help for how to handle errors

New developer workflow:

  • Option 1: Use an editor with Eslint plugin support, and enable auto-fix on save. (more info)
  • Option 2: Manually run 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

@jridgewell
Copy link
Contributor

jridgewell commented Mar 7, 2019

The most egregious to me: prettier/prettier#5588

@vjeux
Copy link

vjeux commented Mar 14, 2019

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.

@vjeux
Copy link

vjeux commented Mar 14, 2019

I'm curious, what tooling is using the /*OK*/ annotation?

@jridgewell
Copy link
Contributor

/*OK*/ is our own annotation system. Originally, we used a primitive regex for .foo, and reported warnings. So, putting any comment between the . and foo made the regex not match.

Now, we use eslint and check the leadingComments on the property. But we've stuck with /*OK*/ comments.

@rsimha
Copy link
Contributor Author

rsimha commented Apr 25, 2019

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 master, re-ran the auto-fixer, and pushed a new commit at 1d990a5.

Current status:

  1. Quoted object keys are now being preserved (Respect quoted object keys, as unquoting breaks aggressive minifiers like Closure Compiler prettier/prettier#4327) ✔️
  2. Inline /*OK*/ tags are still being moved across parentheses (link, Inline single-line comments next to parentheses are being moved incorrectly prettier/prettier#5973) ✖️
  3. Typecast parentheses are still being moved incorrectly (link, JSDoc type cast parentheses is removed  prettier/prettier#4799 (comment)) ✖️
  4. HTML template strings are still being reformatted (link, Disable embedded language formatting by default and make it configurable prettier/prettier#5588 (comment)) ✖️

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
/cc @vjeux for visibility

@rsimha
Copy link
Contributor Author

rsimha commented Apr 25, 2019

I ran some checks on the new code.

  • Item 2 (inline /*OK*/ tags) affects just 4 or 5 function calls, so I think we can work around it. See the output from gulp presubmit
  • Item 3 (typecast parentheses) is a blocker. The output from gulp check-types detects only a subset of incorrect typecasts.
  • Item 4 (HTML template strings) can be worked around by using .trim(), but we'd need a mechanism to enforce its usage.

@jridgewell
Copy link
Contributor

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.

@rsimha
Copy link
Contributor Author

rsimha commented May 14, 2019

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.

3p/3p.js Outdated Show resolved Hide resolved
@rsimha
Copy link
Contributor Author

rsimha commented May 14, 2019

This PR is now ready for a full review. Some notes:

  • The overwhelming majority of changes come from the commit titled First pass of prettier across entire codebase (d34188b), which contains the automatic fixes applied by prettier. This commit can be skimmed through, since we've eliminated breaking changes in the above comments.
  • The rest of the commits contain manual changes made by me (see PR description), and should be reviewed as usual.

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 HEAD, rerun prettier, and make sure Travis is green before merging. (I'll likely do this late on Friday evening.)

@newmuis
Copy link
Contributor

newmuis commented May 15, 2019

/cc @gmajoulet @Enriqe can you take a look for stories while I'm out?

@Enriqe
Copy link
Contributor

Enriqe commented May 15, 2019

@newmuis sure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants