https://pantsbuild.org/ logo
Join Slack
Powered by
:fishing_pole_and_fish: for feedback on my `fmt`+`...
# development
b
🎣 for feedback on my `fmt`+`lint` use the same process proposal: https://docs.google.com/document/d/1KqrPP6VVi-Kq8EUqfo2IFffaHxdysCMPwrInoewM-LM/edit?usp=sharing
w
🚢
✅ 2
b
I've just scratched the start of this. I think the main change will be a function added in Rust to "diff" two snapshots. It'll be a challenging little Rust puzzle to chew on. The "diff" will likely return 
(LHS unique, RHS unique, exists-in-both-but-different)
as that is generic enough to be useful elsewhere, solves this case, and isn't tooooo hard to write
It compiles 😳
Oh wow this is coming along nicely. I'm sure I'll have tons of fun wiring this up to the existing rules, but sneak preview on the entirety of the new formatter rules!
Copy code
@rule(level=LogLevel.DEBUG)
async def get_docformatter_process(request: DocformatterProcessRequest, docformatter: Docformatter) -> Process:
    docformatter_pex = await Get(VenvPex, PexRequest, docformatter.to_pex_request())

    process = await Get(
        Process,
        VenvPexProcess(
            docformatter_pex,
            argv=[
                "--in-place",
                *docformatter.args,
                *request.snapshot.files,
            ],
            input_digest=request.snapshot.digest,
            output_files=request.snapshot.files,
            description=(
                f"Run Docformatter on {pluralize(len(request.field_sets), 'file')}."
            ),
            level=LogLevel.DEBUG,
        ),
    )
    return process
w
awesome! the rust side of this hopefully got a bit easier via https://github.com/pantsbuild/pants/issues/13112 … did you implement diffing via 
DigestTrie
?
b
I haven't seen that issue 👀 I did implement if on 
DigestTrie
Although I just bumped into 
scalafmt
which formats in partitions 🤯 This might throw a wrench in my approach
w
I haven’t seen that issue 👀 I did implement if on 
DigestTrie
yea, cool… otherwise it would have been recursive database access, heh.
Although I just bumped into 
scalafmt
which formats in partitions
it does, yea… but it should still emit something that can be merged into a single Digest at the end to be committed (or not)
b
Yeah, unfortunately my refactor naively assumed there was a 1:1 between a 
fmt
request and a 
Process
.
I'm thinking now I'm actually doing a refactor and a change at the same time, and there might be a way to do them seperately
Context & Brain dump: • We need to change the 
message
on 
FmtResult
to print the changed files and not stdout/stderr (those get logged) • To do that we need to (maybe) diff snapshots, but a 
ProcessResult
has a 
Digest
, so we need to request a 
Snapshot
but the helper method(s) can't do 
async/await
• So I went the route of refactor so the formatters rules just return the process, then 
fmt.py
can do whatever it wants
This all seems little silly given it's just the message, but 🤷‍♂️
I think I see a path forward, however
It does involve some boilerplate being moved into the tools. Even more motivation for me to wrap up https://github.com/pantsbuild/pants/pull/14238
w
@bitter-ability-32190: it would likely be easier to not stream the diff (via FmtResult), and instead only calculate a diff it when all formatters had completed?
But also, FmtResult holding Snapshots rather than Digests should be a relatively minor change if necessary
b
Well the other half of this is that we're invoking the formatter's rules in 
lint
But also, FmtResult holding Snapshots rather than Digests should be a relatively minor change if necessary
Yeah that's the escape lever I'm going to use for now. Unfortunately it forces callers to 
await
a 
Snapshot
from the process output. It's no performance overhead because 
fmt
does this anyways. The only downside is the boilerplate. Which could very well be cleaned up later Yeah thats the shortcut way of getting this working. Because we can't
w
Yea, got it.
b
The "other" refactor in question splits 
StyleRequest
into it's 2 children, and removes 
prior_formatter_result
in favor of just giving the formatter rules a 
snapshot
. I think it'd be one less 
await
per formatter.
(+ simplification)
w
Yeah, unfortunately my refactor naively assumed there was a 1:1 between a 
fmt
request and a 
Process
.
it might still be possible to go this approach with a wrapper for a list of `Process`es to execute… i expect that the bigger question is just whether there are other post-processing steps that a tool might want to take which would require other manipulation of outputs before giving it to the next formatter / linter
☝️ 1
b
Oh duh, thats a good point on the list-of-processes 🤦‍♂️
Yeah I think the refactors could be written piecemeal. Perhaps there's room for a formatter rule like that, but if possible, it'd be nice to make the simple case easy
Diffing PR is live! Really proud of myself for this one: https://github.com/pantsbuild/pants/pull/14872
🦀 1
Open in Slack
PreviousNext
Powered by