https://pantsbuild.org/ logo
#development
Title
# development
b

bitter-ability-32190

03/09/2022, 4:42 PM
🎣 for feedback on my `fmt`+`lint` use the same process proposal: https://docs.google.com/document/d/1KqrPP6VVi-Kq8EUqfo2IFffaHxdysCMPwrInoewM-LM/edit?usp=sharing
w

witty-crayon-22786

03/09/2022, 5:12 PM
🚢
2
b

bitter-ability-32190

03/20/2022, 2:59 PM
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

witty-crayon-22786

03/21/2022, 7:30 PM
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

bitter-ability-32190

03/21/2022, 7:37 PM
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

witty-crayon-22786

03/21/2022, 7:40 PM
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

bitter-ability-32190

03/21/2022, 7:41 PM
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

witty-crayon-22786

03/21/2022, 8:07 PM
@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

bitter-ability-32190

03/21/2022, 8:09 PM
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

witty-crayon-22786

03/21/2022, 8:11 PM
Yea, got it.
b

bitter-ability-32190

03/21/2022, 8:13 PM
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

witty-crayon-22786

03/21/2022, 8:24 PM
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

bitter-ability-32190

03/21/2022, 8:25 PM
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