FmtResult now operates on Snapshots #14865
Conversation
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
| output_snapshot = await Get(Snapshot, Digest, result.output_digest) | ||
| return FmtResult( | ||
| setup.original_snapshot, | ||
| output_snapshot, | ||
| stdout=result.stdout.decode(), | ||
| stderr=result.stderr.decode(), | ||
| formatter_name=request.name, |
There was a problem hiding this comment.
It would probably still be good to have the helper method take the result so that it can decode the stdout/stderr for you at the least (although obviously a bit redundant that the Snapshot needs to be precalculated... could validate that the argument Snapshot has a Digest equal to the result's Digest...?)
There was a problem hiding this comment.
The stdout/stderr isn't always decoded. Sometimes it is passed through strip_v2_chroot. See
There was a problem hiding this comment.
Sure, but whether that happened used to be handled by the optional strip_chroot_path=True, param. Fine either way though.
Co-authored-by: Stu Hood <[email protected]>
|
I think it'd be good to add back the classmethod. It's meant to help with DRY and seems like removing it results in unnecessary churn for plugin others. Bump on the |
I'm not sure it'd really DRY that much, but I could be in the minority. Instead I think a FmtResult(
setup.original_snapshot,
output_snapshot,
stdout=result.stdout.decode(),
stderr=result.stderr.decode(),
formatter_name=request.name,
)
# vs
FmtResult.from_process_result(
result,
original_snapshot=setup.original_snapshot,
output_snapshot=output_snapshot,
formatter_name=request.name,
# strip_chroot_path=True # optional
) |
In order to handle the upcoming change to "unify" both
fmtandlintprocesses for formatters, we need to have themessageofFmtResultbe based on a diff of 2Snapshots. Sincemessagecan't participate inasync/await, we need to changeFmtResultso it already has those snapshots available (namelyinputandoutput).This is unfortunately boilerplate for the callers, but that can potentially be cleaned up later a myriad of ways.
This also should be essentially no overhead as
fmt.pywas already doing this query (although it was doing it conditionally. I suspect doing it unconditionally will have very little impact on perf because this is guaranteed to be cached if the output digest differs from the input).[ci skip-rust]