https://pantsbuild.org/ logo
Join Slack
Powered by
How do I tell MyPy "No really, this isn't `None`, ...
# development
b
How do I tell MyPy "No really, this isn't 
None
, I want to return it and I don't want to propagate the 
Optional
in my return type"
h
# type: ignore[error-code]
w
or 
cast
, right? is the type-ignore preferred?
👍 1
b
That's going to be a lot of annoying ignores to sprinkle around. Maybe I should just make it not-optional and use the ignore comment in the one place where it's constructed as 
None
Or maybe there's a more legitimate issue in my type design that should be fixed. It's 
FallibleJavaSourceDependencyAnalysisResult
here: https://github.com/pantsbuild/pants/pull/12890/files#diff-a3ed131ee51e1222afe5bb72399706ad9aa596af614fc1237c2e7e5f3d19a498
e
Can you just flip your test to 
not None
instead of testing the exit code? The real answer here would be for rule return types to support Union, but they don't.
b
I could test both but it would be really awkward. I don't love testing 
not None
and just assuming that implies a non-zero exit code, although it would work
e
Or maybe just make 
FallibleJavaSourceDependencyAnalysisResult
have a single union field.
Do you really need both or is one or the other fine?
b
Hm, good question
A single union field might be best
e
Cool. If if one or the other is not fine, you can always enrich the right or left arm with a wrapper type with the extras actually needed.
b
I ended up figuring that there wasn't really any point in having 
analysis
in the fallible result at all, I'd might as well have that only hold the process result, then having the enriching rule encapsulate the business logic of extracting the digest from a successful process execution
Tangentially related, is there a reason that we don't have a rule for this conversion?
Copy code
result = await Get(ProcessResult, FallibleProcessResult, fallible_result.process_result)
I naively tried doing that but couldn't convince the engine. When I went and looked for the rule, it kind of exists, but it wants an extra parameter that I don't have or think I need
I suppose I would also be happy with a 
raise_process_execution_if_failed
method on 
FallibleProcessResult
w
yea, it’s unnecessarily complicated. @average-vr-56795 has out https://github.com/pantsbuild/pants/pull/12725 to fix that
b
nice, thanks
a
Which reminds me, I should really respond to those comments - @witty-crayon-22786 were you thinking to just use the existing 
Process.platform
field as the basis for the runner-selection, or needing a new field?
w
@average-vr-56795: the existing 
Process.platform
seems fine i think?
it’s also totally fine to punt: the patch is appreciated regardless
Open in Slack
PreviousNext
Powered by