mozilla-services / screenshots

Firefox Screenshots: the best way to take screenshots on the web.
https://screenshots.firefox.com
Mozilla Public License 2.0
620 stars 128 forks source link

Apply prettier to code #2148

Open ianb opened 7 years ago

ianb commented 7 years ago

I like prettier in concept, as a JavaScript reformatter. I'd be curious what it does, and if we can apply it generally across the codebase.

If it seems at all hard we should not put much effort into this.

ianb commented 7 years ago

This doesn't feel very useful to worry about right now.

johngruen commented 6 years ago

Having just worked on SS for the first time in awhile, i felt like line-length in or code base greatly decreased legibility. Might be work reconsidering this

jaredhirsch commented 6 years ago

This could be cool. Note that any changes will need to pass the mozilla eslint rules, since exported code goes into mozilla-central :+1:

pdehaan commented 6 years ago

@johngruen Did you want all of prettier's formatting, or just a more managed maximum line length?

ESLint does have a max-len rule which may do a bit of what you want.


UPDATE: This may actually do what you want, but produce a pretty sizable diff:

$ npx prettier "{bin,docs,server,static,test,webextension}/**/*.{css,js,json,md,scss,yml}" "*.md" --write
$ npm run lint:js

And it looks like everything still passes ESLint.

$ git status ```sh $ git status On branch master Your branch is up-to-date with 'origin/master'. Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git checkout -- ..." to discard changes in working directory) modified: CHANGELOG.md modified: CONTRIBUTORS.md modified: README.md modified: bin/build-scripts/make_versions_exact.js modified: bin/build-scripts/substitute-env.js modified: docs/METRICS.md modified: docs/acceptance.md modified: docs/deployment.md modified: docs/error-handling.md modified: docs/export-to-firefox.md modified: docs/running-mochitests.md modified: docs/server-release.md modified: docs/takedown.md modified: docs/testing-the-api.md modified: server/src/ab-tests.js modified: server/src/browser-send-event.js modified: server/src/caching.js modified: server/src/config.js modified: server/src/db.js modified: server/src/dbschema.js modified: server/src/delete-shot-button.js modified: server/src/errors.js modified: server/src/events.js modified: server/src/footer-view.js modified: server/src/ga-activation.js modified: server/src/header.js modified: server/src/jobs.js modified: server/src/l10n.js modified: server/src/linker.js modified: server/src/logging.js modified: server/src/middleware/csrf.js modified: server/src/middleware/l10n.js modified: server/src/oembed-view.js modified: server/src/pages/creating/model.js modified: server/src/pages/creating/page.js modified: server/src/pages/creating/view.js modified: server/src/pages/homepage/controller.js modified: server/src/pages/homepage/homepage-header.js modified: server/src/pages/homepage/model.js modified: server/src/pages/homepage/page.js modified: server/src/pages/homepage/view.js modified: server/src/pages/leave-screenshots/model.js modified: server/src/pages/leave-screenshots/page.js modified: server/src/pages/leave-screenshots/server.js modified: server/src/pages/leave-screenshots/view.js modified: server/src/pages/metrics/model.js modified: server/src/pages/metrics/page.js modified: server/src/pages/metrics/server.js modified: server/src/pages/metrics/view.js modified: server/src/pages/not-found/model.js modified: server/src/pages/not-found/page.js modified: server/src/pages/not-found/view.js modified: server/src/pages/settings/controller.js modified: server/src/pages/settings/model.js modified: server/src/pages/settings/page.js modified: server/src/pages/settings/view.js modified: server/src/pages/shot/color-picker.js modified: server/src/pages/shot/controller.js modified: server/src/pages/shot/crop-tool.js modified: server/src/pages/shot/drawing-tool.js modified: server/src/pages/shot/editor-history.js modified: server/src/pages/shot/editor.js modified: server/src/pages/shot/footer.js modified: server/src/pages/shot/highlighter-tool.js modified: server/src/pages/shot/model.js modified: server/src/pages/shot/page.js modified: server/src/pages/shot/pen-tool.js modified: server/src/pages/shot/promo-dialog.js modified: server/src/pages/shot/server.js modified: server/src/pages/shot/shotpage-header.js modified: server/src/pages/shot/text-tool.js modified: server/src/pages/shot/time-diff.js modified: server/src/pages/shot/view.js modified: server/src/pages/shotindex/controller.js modified: server/src/pages/shotindex/footer.js modified: server/src/pages/shotindex/model.js modified: server/src/pages/shotindex/myshots-header.js modified: server/src/pages/shotindex/page.js modified: server/src/pages/shotindex/server.js modified: server/src/pages/shotindex/view.js modified: server/src/proxy-url.js modified: server/src/ravenclient.js modified: server/src/reactrender.js modified: server/src/reactruntime.js modified: server/src/reactutils.js modified: server/src/responses.js modified: server/src/server.js modified: server/src/servershot.js modified: server/src/share-buttons.js modified: server/src/signin-button.js modified: server/src/statsd.js modified: server/src/users.js modified: server/src/watchdog.js modified: server/views-docs.md modified: test/addon/browser_screenshots_ui_check.js modified: test/addon/head.js modified: test/server/unit/l10n-test.js modified: test/server/unit/middleware/l10n-test.js modified: test/server/unit/pages/homepage/model-test.js modified: test/test-ab-test.js modified: test/test.js modified: test/unit/selection.js modified: webextension/.eslintrc.js modified: webextension/README.md modified: webextension/background/README.md modified: webextension/background/analytics.js modified: webextension/background/auth.js modified: webextension/background/communication.js modified: webextension/background/deviceInfo.js modified: webextension/background/main.js modified: webextension/background/selectorLoader.js modified: webextension/background/senderror.js modified: webextension/background/startBackground.js modified: webextension/background/takeshot.js modified: webextension/blobConverters.js modified: webextension/catcher.js modified: webextension/clipboard.js modified: webextension/domainFromUrl.js modified: webextension/experiments/screenshots/api.js modified: webextension/makeUuid.js modified: webextension/onboarding/slides.js modified: webextension/randomString.js modified: webextension/selector/README.md modified: webextension/selector/callBackground.js modified: webextension/selector/documentMetadata.js modified: webextension/selector/shooter.js modified: webextension/selector/ui.js modified: webextension/selector/uicontrol.js modified: webextension/selector/util.js modified: webextension/sitehelper.js ```
jaredhirsch commented 6 years ago

opened #4916 to discuss just adding the eslint rule. @pdehaan did you see any particularly nice examples of the before/after diff with prettier applied?

pdehaan commented 6 years ago

@6a68 It's a pretty massive diff, but I didn't see anything too amazing that would cause me to find equivalent ESLint rules to improve readability.

I ended up playing briefly w/ prettier CLI flags to try and get the prettier formatting as close to the current formatting as possible, but noticed that there wasn't always consistent spacing around parens, or usage of optional parens on arrow functions, etc. So I think in the case of overall code consistency prettier would really help. But there were also a few places where prettier's formatting wasn't as nice. Maybe if there were really offensive reformatting I think you can tell prettier to ignore specific lines/blocks of code. 🤷‍♂️

Here was my "final" $ prettier command:

npx prettier "{bin,docs,server,static,test,webextension}/**/*.{css,js,json,md,scss,yml}" "*.md" --write --arrow-parens=avoid --trailing-comma=all --print-width=110 --bracket-spacing

We could also add a .prettierignore file (or possibly reuse .eslintignore) if there were also certain l10n paths that we wanted to ignore. No point in reformatting a bunch of JSON l10n blobs. Which is also why I didn't bother trying to format the root *.json files -- since I wanted to avoid touching package.json and more importantly, package-lock.json.

pdehaan commented 6 years ago

This may also be useful: https://www.npmjs.com/package/pretty-quick We could run prettier only against staged files using git hooks.