if not present, prepend @format docblock

samouri commented 7 years ago

Inspired by the adoption at Facebook: mark every prettified file with /** @format */.

Works in tandem with: https://github.com/Automattic/wp-calypso/pull/16394

Note: the thing I don't like about this is its currently breaking every test because it adds the docblock to all of them...

jsnajdr commented 7 years ago

This could be useful for all users of Prettier, not only for Calypso -- what do you think about submitting a PR to the official version? There could be a --prepend-format-comment option or something like that.

The fewer changes our fork has, the better.

samouri commented 7 years ago

There could be a --prepend-format-comment option or something like that.

Right now the benefit is only for those trying to adopt it slowly within a large codebase. If standard tooling around the flag @format ever enters the picture then i could see this more useful for the community. I could also see exposing a new function from the prettier api as well (containsFormatFlag( txt ) --> bool) for people to use in their pre-commit hooks. I'll see if i can find the spare cycles to open this idea up with the prettier folks.

No matter how that goes though, we shouldn't slow down adding this functionality to our fork! I only needed to modify a single line of prettier code to add this :)

samouri commented 7 years ago

@jsnajdr pointed me towards this package for docblock parsing that looks promising! https://github.com/facebook/jest/blob/master/packages/jest-docblock/src/index.js

Things to change before merging:

[x] explore using jest-docblock for docblock parsing
[x] add in option for --prepend-format-comment
[x] move prependIfNecessary to after formatRange backs out so that we don't add docblock for formatRange calls

dmsnell commented 7 years ago

Can you help us understand why we want to add this to our normal process instead of making it a one-off operation? Is the goal to continually add the formatting indicator to every file? how is that different than just running prettier over the entire codebase?

samouri commented 7 years ago

@dmsnell: did you read the Facebook adoption summary? We'd want to do this incrementally for the same reasons Facebook is doing this:

it's not feasible to just convert everything in one go and to convince everyone that their entire codebase is going to be reformatted under their feet. So we need to find a more incremental approach.

Running prettier on a piece of code is a pretty expensive operation, it makes your pull request look bad because of a lot of unrelated changes and it causes merge conflicts for all the outstanding pull requests. So once a file has been formatted, you should do everything to make sure it remains formatted.

wp-calypso is worked on by ~300 people. a unilateral shift in workflow would be an unreasonable request. as @jsnajdr hinted at, this is a change that could be useful to incorporate into prettier proper for the wider community to use:

embedding the @format into the code allows for standard tooling (and some already have been made) to start supporting the pragma and only function on files with the identifier
there are probably many other large codebases with similar needs. It would have been nice for Facebook to contribute this back to the ecosystem

samouri commented 7 years ago

ready for review! tests all pass when commenting out our defaults override (except for the couple about extra added params)

Automattic / wp-prettier

if not present, prepend @format docblock #6