Skip to content

chore(various): Autoformat JS files #4726

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

Merged
merged 11 commits into from
Mar 17, 2022

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Mar 16, 2022

This applies the autoformatter to all of the JS (as in, written originally in JS, not transpiled into JS) files in the repo, to avoid having future changes to those files polluted by irrelevant autoformating triggered by saving the file. A few non-JS files, primarily in the ember package, are also included.

@lobsterkatie lobsterkatie force-pushed the kmclb-autoformat-js-files-march-2022 branch from 2de701f to bd319d4 Compare March 16, 2022 18:29
@lobsterkatie lobsterkatie force-pushed the kmclb-autoformat-js-files-march-2022 branch 2 times, most recently from 8a5ecfe to 0251b73 Compare March 17, 2022 01:26
@github-actions
Copy link
Contributor

github-actions bot commented Mar 17, 2022

size-limit report

Path Base Size (72aed62) Current Size Change
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.49 KB 19.49 KB -0.01% 🔽
@sentry/browser - ES5 CDN Bundle (minified) 62.17 KB 62.17 KB 0%
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.11 KB 18.11 KB 0%
@sentry/browser - ES6 CDN Bundle (minified) 55.5 KB 55.5 KB 0%
@sentry/browser - Webpack (gzipped + minified) 22.41 KB 22.41 KB 0%
@sentry/browser - Webpack (minified) 76.82 KB 76.82 KB 0%
@sentry/react - Webpack (gzipped + minified) 22.45 KB 22.45 KB 0%
@sentry/nextjs Client - Webpack (gzipped + minified) 46.62 KB 46.62 KB 0%
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.36 KB 25.36 KB 0%
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 23.72 KB 23.72 KB 0%

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me. Just left a comment about a change I noticed which seemed interesting to me. That's probably more a point of discussion though than anything else.

nock('http://example.com')
.get('/')
.reply(200, 'ok');
nock('http://example.com').get('/').reply(200, 'ok');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting to me. Are we fine with that change? I might be wrong but to me it seems like this chain was spread over multiple lines on purpose. There are some similar reformats with arguments, array contents, etc. which were also reformatted to a single line.

I'm not criticising the change itself and I know that argument/parameter wrapping and alignment is hard to define properly for every situation but I just wanted to highlight that this might be an unintended format change. Nevertheless, I think consistency is important so I'd be totally on board with the change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually happen to agree with you that it's easier to read on multiple lines. That said, it's not up to us, or at least, as long as we're using Prettier it's not - it will make the change whether we want it to or not (and that's not configurable - see below). And since we format on save, trying to keep it in a different state than Prettier wants it is ultimately a losing game. (I know, I've tried!) This PR just keeps those inevitable changes from ending up in unrelated PRs because someone happens to have touched one of these files and hit 'save.'

Here's Prettier's take on why it should get to choose:

https://prettier.io/docs/en/why-prettier.html
https://prettier.io/docs/en/option-philosophy.html

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for the links. They're an interesting read. So as I've said, the PR is looking good to me.

@lobsterkatie lobsterkatie merged commit a7a6ea5 into master Mar 17, 2022
@lobsterkatie lobsterkatie deleted the kmclb-autoformat-js-files-march-2022 branch March 17, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants