-
-
Notifications
You must be signed in to change notification settings - Fork 342
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
chore: add prettier to pre-commit for markdown and json files #4846
Conversation
5025d38
to
969e10c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4846 +/- ##
=============================================
+ Coverage 92.165% 92.258% +0.092%
=============================================
Files 658 658
Lines 77223 77229 +6
Branches 27169 27957 +788
=============================================
+ Hits 71173 71250 +77
+ Misses 5955 5883 -72
- Partials 95 96 +1
... and 25 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0ffe199 | 1209.51 ms | 1223.94 ms | 14.43 ms |
4597906 | 1222.88 ms | 1235.75 ms | 12.88 ms |
32ac934 | 1238.86 ms | 1243.37 ms | 4.52 ms |
2df93b0 | 1210.73 ms | 1228.76 ms | 18.02 ms |
bbcbaff | 1216.82 ms | 1242.34 ms | 25.52 ms |
0f4071f | 1212.80 ms | 1239.22 ms | 26.43 ms |
3297d6e | 1228.57 ms | 1255.04 ms | 26.47 ms |
4ea2c00 | 1216.19 ms | 1237.12 ms | 20.93 ms |
44ce888 | 1208.98 ms | 1224.72 ms | 15.74 ms |
1b69ee7 | 1246.08 ms | 1257.71 ms | 11.63 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
0ffe199 | 20.76 KiB | 436.50 KiB | 415.74 KiB |
4597906 | 21.58 KiB | 678.19 KiB | 656.61 KiB |
32ac934 | 21.58 KiB | 616.72 KiB | 595.14 KiB |
2df93b0 | 21.58 KiB | 682.40 KiB | 660.81 KiB |
bbcbaff | 22.85 KiB | 414.09 KiB | 391.24 KiB |
0f4071f | 21.58 KiB | 681.72 KiB | 660.14 KiB |
3297d6e | 21.58 KiB | 418.45 KiB | 396.86 KiB |
4ea2c00 | 21.58 KiB | 706.47 KiB | 684.88 KiB |
44ce888 | 22.85 KiB | 414.65 KiB | 391.80 KiB |
1b69ee7 | 21.58 KiB | 707.43 KiB | 685.84 KiB |
Previous results on branch: philprime/updated-formatting-tools
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
36e2624 | 1229.09 ms | 1258.04 ms | 28.96 ms |
bc15a32 | 1216.94 ms | 1238.37 ms | 21.43 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
36e2624 | 22.32 KiB | 819.55 KiB | 797.24 KiB |
bc15a32 | 22.32 KiB | 819.55 KiB | 797.23 KiB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing the benefit of this. Did you run json and md linting over the whole codebase already? Because the only thing I see changed are asset catalogs in the sample apps. It's a heavyweight change bringing in NPM and doesn't appear to have made any material improvements.
@armcknight Yes, I already fixed most of the files in #4837, where you can see that there were quite a lot of formatting errors like indents, double whitespaces etc. I am working with VSCode/Cursor using Prettier as the default formatter. As soon as I save the CHANGELOG.md, it keeps formatting. Of course this is behavior on my machine, and I can disable it, but simply due to the fact that the default configuration of the prettier formatter is applying changes is indicator enough that there's room for improvement. This PR is a follow-up based on the proposition of @philipphofmann in his comment. I do agree with you that NPM is heavy weight, but AFAIK there's no other way to get prettier enabled. https://github.com/getsentry/sentry-docs/blob/master/.pre-commit-config.yaml#L26-L32 Regarding only changing the We can introduce it as a CI-only dependency or add an additional "Sentry-Contributor" init command to keep it more light weight. |
On a side-note, with the introduction of test plans in #4848, we are adding more JSON documents over time, as it's the format used by test plans. |
Ah I see, thanks for linking me to that PR. I don't feel strongly about not bringing in npm, and since it's preinstalled in CI, that shouldn't increase CI times significantly. Although I would say, I would prefer to exclude anything generated by Xcode, like the manifests for asset catalogs or test plans. The test resources you currently have excluded, I think should be linted, since those are hand edited and maintained by us. And like you said, we can exclude the ones that are supposed to be broken in some way (maybe those can be put in a subdirectory to make that easier). |
I'll change the PR to format test resources, and exclude the garbage ones 👍 Regarding the Xcode files, I don't fully understand why we should exclude them. Even though they are generated by Xcode, I believe we only need to format them once and they are good to go. Do I miss something here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still need to fix the test_ReadingBigFile and test_WritingBigFile tests cause the linter modified the big file for the tests 😄
At first glance I like woaaa 🤯
The PR will modify the git history of so many files, but only for files for which the GH history isn't relevant. So LGTM. I prefer having things consistent.
We could split the PR in one configuring everything and one to fix issues. Might make it easier to look into it in the future |
I think that's a chicken-egg problem and not worth the effort. You set up everything and you disable most of it, only to enable it again in another PR. I'm fine with doing all in one PR unless you disagree. |
#skip-changelog