Skip to content

Enforce formatting of TOML files #30128

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 5 commits into from
Sep 19, 2023
Merged

Enforce formatting of TOML files #30128

merged 5 commits into from
Sep 19, 2023

Conversation

sagudev
Copy link
Member

@sagudev sagudev commented Aug 18, 2023

Formatting is enforced by CI rather than requiring users to install taplo as I suggested in #26728 (comment)


  • These changes do not require tests because they are helper scripts

@sagudev sagudev marked this pull request as draft August 18, 2023 12:42
@sagudev sagudev force-pushed the toml-tidy branch 2 times, most recently from 4014164 to e57e803 Compare August 18, 2023 13:03
@sagudev sagudev marked this pull request as ready for review August 18, 2023 13:24
Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

I think this will make things tricky for developers because bootstrap isn't installing taplo, so the changes will fail on CI but not locally with test-tidy. Can we install a toml formater via pip or cargo dependencies that does the right thing?

@sagudev
Copy link
Member Author

sagudev commented Aug 21, 2023

Can we install a toml formater via pip or cargo dependencies that does the right thing

Is there any place where I can put cargo install taplo-cli --locked without doing separately for every platform?

@mrobinson
Copy link
Member

Can we install a toml formater via pip or cargo dependencies that does the right thing

Is there any place where I can put cargo install taplo-cli --locked without doing separately for every platform?

Could this be made a dev dependency of the winit port?

@sagudev
Copy link
Member Author

sagudev commented Aug 21, 2023

Can we install a toml formater via pip or cargo dependencies that does the right thing

Is there any place where I can put cargo install taplo-cli --locked without doing separately for every platform?

Could this be made a dev dependency of the winit port?

Could we run cli from dev-dependencies? I think that is not possible: rust-lang/cargo#2267

@mrobinson
Copy link
Member

It seems a tool like toml-sort could be installed by pip and then run unconditionally. What do you think?

@sagudev
Copy link
Member Author

sagudev commented Sep 15, 2023

It seems a tool like toml-sort could be installed by pip and then run unconditionally. What do you think?

Hm, it certainly is more convenient to install via mach. I will see if I can make config for it.

@mrobinson
Copy link
Member

Great. That sounds like a plan. I can help out with the integration into "./mach fmt" as well.

@sagudev
Copy link
Member Author

sagudev commented Sep 15, 2023

After reading toml-sort usage manual I am not so sure it was meant to be used this way. In taplo you can just do taplo fmt and it would automatically search for config and formatted all files. In toml-sort you need to do toml-sort --in-place **.toml if you have bash glob enabled. There is also no way to really exclude some toml files. While we could write wrappers in mach that would do all this stuff, I do not think that is the way to go.

I think Taplo is more oriented towards users like us, that want to enforce formatting in their project and is easier to be used as standalone, while toml-sort is just utility that works for files being piped rather than on projects.

@mrobinson
Copy link
Member

Hrm. Maybe we can build taplo into the target directory or install it for the user with cargo during bootstrapping?

@sagudev
Copy link
Member Author

sagudev commented Sep 15, 2023

I am working on user installation with cargo during bootstrapping.

Wow how mach has changed since the last time I touched it (well bootstrapping to be exact).

@sagudev sagudev force-pushed the toml-tidy branch 3 times, most recently from 467e1f8 to 0470cc9 Compare September 16, 2023 06:46
@sagudev sagudev requested a review from mrobinson September 16, 2023 06:59
Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Thanks so much for iterating on this. This is looking really good. I have a suggestion for a few minor changes.

@sagudev sagudev force-pushed the toml-tidy branch 2 times, most recently from 5c511e4 to e2af6ef Compare September 16, 2023 08:17
@sagudev sagudev requested a review from mrobinson September 16, 2023 08:18
Comment on lines +74 to +78
- name: Install taplo
uses: baptiste0928/cargo-install@v2
with:
crate: taplo-cli
locked: true
Copy link
Member

Choose a reason for hiding this comment

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

Will this be installed by ./mach bootstrap below now?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be, but this way it's faster as this action will cache it. Using cargo install takes 4-5min.

Comment on lines +63 to +67
- name: Install taplo
uses: baptiste0928/cargo-install@v2
with:
crate: taplo-cli
locked: true
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple little nits before landing. Thanks!

Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Thank you!

@sagudev sagudev enabled auto-merge September 19, 2023 08:17
@sagudev sagudev added this pull request to the merge queue Sep 19, 2023
Merged via the queue into servo:master with commit 7caac97 Sep 19, 2023
@sagudev sagudev deleted the toml-tidy branch September 19, 2023 10:15
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.

Enforcing formatting of TOML files
2 participants