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
[FMT] apply toml formatting hook on pyproject.toml #3872
Conversation
👋 @Remi-Gau Thanks for creating a PR! Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft. Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
The default config of this formater sorts the content of the toml alphabetically which has its pros and cons |
Can check if there is more config option to avoid this reformatting |
Codecov Report
@@ Coverage Diff @@
## main #3872 +/- ##
=======================================
Coverage 91.60% 91.60%
=======================================
Files 134 134
Lines 15705 15705
Branches 3270 3270
=======================================
Hits 14386 14386
Misses 770 770
Partials 549 549
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
.pre-commit-config.yaml
Outdated
rev: v2.10.0 | ||
hooks: | ||
- id: pretty-format-toml | ||
args: [--autofix, --indent, '2'] |
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.
Any particular reason why 2 spaces instead of 4 ? I can't find a good, accepted style guide for TOML, so I'd think this is just developer preference.
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 honestly went with what was in the read of the pre-commit hook repo? You want 4, let's do 4.you want 8, let's do 8. You want 16... Get me a larger screen.
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.
😂 Just wanted to confirm that there wasn't a style guide I was missing ! I'm fine with 2 or 4.
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.
given that our code uses 4, let's go with 4 to avoid the mental gymnastic of 4 for python, 2 for toml (my brain is too old and unflexible for those exercises)
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.
LGTM so far.
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 do prefer the original order for some tables like project
but I'm not too bothered by it. LGTM otherwise, thx!
OK let's get this in then |
Closes #3870
Changes proposed in this pull request:
Advantages
Drawback