Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Taplo toml formatter #18865

Merged
merged 8 commits into from May 12, 2023
Merged

Add Taplo toml formatter #18865

merged 8 commits into from May 12, 2023

Conversation

bryanwweber
Copy link
Contributor

@bryanwweber bryanwweber commented Apr 30, 2023 •
edited

This PR adds Taplo as a downloaded tool to format TOML files. As currently implemented, there are two rules, one for toml_sources and one specifically for pyproject.toml files. @thejcannon suggested making a PR to talk about a targetless/hybrid formatter for TOML files. I can't dig up the Slack thread at the moment but I'll add it when I can! Slack thread: https://pantsbuild.slack.com/archives/C046T6T9U/p1680552392695329

@stuhood stuhood requested a review from thejcannon May 1, 2023 20:41
@thejcannon
Copy link
Member

Can you link the thread that talked about why we need this as a target formatter? I don't see anything specific to targets other than skip.

@bryanwweber
Copy link
Contributor Author

@thejcannon I just edited the OP with the thread. The main thing is that if I have a pyproject.toml file which is the source for a python_requirements() target, or if I have a files() target that's included in a python_distribution() to build a wheel, I'd still like pyproject.toml to be formatted by taplo. In addition, I'd like explicit toml_source[s]() targets to be formatted. Although there's not much reason to have a toml_source right now, other than to be able to point the formatter at it.

@thejcannon
Copy link
Member

thejcannon commented May 3, 2023 •
edited

So with targetless formatters/fixers they simply work on globs. They also don't not work on files-that-have-targets (double negative). Case and point is preamble plugin we use in this repo. It formats the Python files through the configured globs, and doesn't care if there's a target or not.

So check out preamble plugin as inspiration, although I think your globs can just come from a config.

@bryanwweber
Copy link
Contributor Author

Thanks @thejcannon! Would there be a way to exclude certain files from formatting then? Just by the glob?

@thejcannon
Copy link
Member

Yup, glob-based excludes (you could use one glob for everything, or separate includes/excludes globs). Kinda nasty but that's the art.

@bryanwweber
Copy link
Contributor Author

bryanwweber commented May 3, 2023 •
edited

OK great, thanks! I'll look at the preamble plugin then. Do you think this plugin would still be valuable for pants overall, should I fix this branch to upstream the capability? Or just maintain this locally?

@thejcannon
Copy link
Member

Oh yes, we love upstreamed plugins. I for one welcome more and more formatters.

Please do continue this PR 😄

thejcannon
thejcannon reviewed May 4, 2023
View reviewed changes
Copy link
Member

@thejcannon thejcannon left a comment

Choose a reason for hiding this comment

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

I'm super stoked for this, thank you for your contribution and your patience!

🚀

src/python/pants/backend/tools/taplo/rules.py Outdated Show resolved Hide resolved
src/python/pants/backend/tools/taplo/subsystem.py Outdated Show resolved Hide resolved
src/python/pants/backend/tools/taplo/subsystem.py Outdated Show resolved Hide resolved
src/python/pants/backend/tools/taplo/rules.py Outdated Show resolved Hide resolved
src/python/pants/backend/tools/taplo/rules.py Outdated
Comment on lines 22 to 30
@rule
async def partition_inputs(
request: TaploFmtRequest.PartitionRequest, taplo: Taplo
) -> Partitions[Any]:
if taplo.skip:
return Partitions()

print(taplo)
return Partitions.single_partition(taplo.filter_inputs(request.files))
Copy link
Member

Choose a reason for hiding this comment

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

I should file myself an issue to make this less boilerplate-y.

src/python/pants/backend/tools/taplo/subsystem.py Outdated Show resolved Hide resolved
thejcannon
thejcannon approved these changes May 9, 2023
View reviewed changes
Copy link
Member

@thejcannon thejcannon left a comment

Choose a reason for hiding this comment

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

Looks great! Minor comments then we can merge.

Thanks again! (And don't forget, open invitation to do the honors of turning this on in this repo 🏆 )

src/python/pants/backend/tools/taplo/subsystem.py Outdated Show resolved Hide resolved
src/python/pants/backend/tools/taplo/subsystem.py Outdated Show resolved Hide resolved
thejcannon
thejcannon approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@thejcannon thejcannon left a comment

Choose a reason for hiding this comment

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

I'll go ahead and merge the nits.

Thanks a million!

@thejcannon
Copy link
Member

Hmm CI seems to be on vacation. Can you merge latest main into this branch?

@thejcannon thejcannon enabled auto-merge (squash) May 12, 2023 01:42
@thejcannon thejcannon merged commit 9465d3d into pantsbuild:main May 12, 2023
17 checks passed
@bryanwweber bryanwweber mentioned this pull request Jun 6, 2023
Merged
kaos pushed a commit that referenced this pull request Jun 6, 2023
@bweber-rebellion
Add taplo to the release (#19258)
2103025 
Per the thread here:
https://pantsbuild.slack.com/archives/C0D7TNJHL/p1685984488435849?thread_ts=1685983060.055239&cid=C0D7TNJHL
after merging the Taplo formatter with #18865 it wasn't added to the
release dependencies. This adds the backend to the file.
github-actions bot pushed a commit that referenced this pull request Jun 6, 2023
@bweber-rebellion @WorkerPants
Add taplo to the release (#19258)
5a9d6b9 
Per the thread here:
https://pantsbuild.slack.com/archives/C0D7TNJHL/p1685984488435849?thread_ts=1685983060.055239&cid=C0D7TNJHL
after merging the Taplo formatter with #18865 it wasn't added to the
release dependencies. This adds the backend to the file.
@WorkerPants WorkerPants mentioned this pull request Jun 6, 2023
Merged
kaos pushed a commit that referenced this pull request Jun 6, 2023
@WorkerPants @bweber-rebellion
Add taplo to the release (Cherry-pick of #19258) (#19259)
735281a 
Per the thread here:
https://pantsbuild.slack.com/archives/C0D7TNJHL/p1685984488435849?thread_ts=1685983060.055239&cid=C0D7TNJHL
after merging the Taplo formatter with #18865 it wasn't added to the
release dependencies. This adds the backend to the file.

Co-authored-by: Bryan Weber <bweber@rebelliondefense.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thejcannon thejcannon thejcannon approved these changes

Assignees
No one assigned
Labels
category:new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bryanwweber @thejcannon @bweber-rebellion