<@U098KD4HJ>: re: using a `CoverageResult` union f...
# development
@dry-analyst-73584: re: using a
union for https://github.com/pantsbuild/pants/pull/8978 : what trouble did you run into?
related: even if there are some python-specific bits in the core
rules, that would still be preferable to a separate goal, i think
I’ll backtrack our conversation with Greg from last week. @hundreds-breakfast-49010 do you happen to remember why we realized it wouldn’t work as expected?
basically, i think that whatever hacks are necessary to allow the flag and goal to live in the right place is critical
We set up a
union, but could not
await Get[CoverageResult]
in the generic test runner because the only way to get a
was in the pants.backend.python backend, which is not always installed.
i expect that the union thing can be figured out, but if in the meantime we need python bits in core, that's ok
This will break pants v2 for anyone without the pants.backend.python backend installed.
Which was why we stopped.
UnionMembership will allow you to check whether you have members: if you don't, you skip coverage (edit: thinking about this more, i can imagine what you ran into)
because we have lots of time to refactor the rules, but if a really different CLI interface is exposed now, there is a definite risk that we'll lose the motivation to fix it
The build graph doesn't know that though - I had the
await Get
inside a check for whether the
option was passed, but the build graph sees that it requests a
and fails.
👍 1
yea, edited the above. it's fine to land with workarounds that allow the options/goal to live in the right place, and a TODO pointing to a ticket.
@dry-analyst-73584: does that make sense?
basically, if you need to put "PythonCoverageResult" in the core, then ok-fine-lets-do-it-for-now with a TODO
(as an Optional field on the TestResult, for example)
we can discuss the union thing independently, because i think i know how it would work.
but it doesn't need to block if the UX is as-designed.
Originally, we did do this. But the issue is that we must provide a no-op way to
await Get[PythonCoverageResult]
is not registered. If we provide a no-op, then the rule graph would always use that rule because it’s the simplest path, rather than having to call the Python rules
Can we rename the goal to be something very hacky and perhaps even frustrating to type so that a) users know this is a WIP and b) we don’t settle with the UX? Something like
we could, but that is less desirable than having rule-internal/privatish hacks
(as an Optional field on the TestResult, for example)
We already have that for an individual
. But we need some way to collect and merge all the `TestResult.coverage_digest`s into one single
. So, we would have to create a new type. This is where we ran into the issues with needing a no-op implementation
ok. that's the using a union as a field case, i think.
i'm not fully understanding what failed in the rule graph, i think. if a union is empty, the
yield Get[X]($union)
"disappears" from the graph... there is no validation, effectively
cf: the test runner itself
so i think there are probably minor tweaks there
@dry-analyst-73584: i won't be able to work on it during the day, but if you post a failing version of this as one commit atop your working PR, i can poke at it a bit after hours?
Sure 🙂 I would be happy to pair on it as well if that's useful, I would love to know how to solve this!
❤️ 1
i won't be able to pair today, unfortunately... i really need to make progress on some other work i've been putting off
but i'll sit in a recorded video chat and talk to myself this evening if anyone is around. either way, can post it
Tonight I have an event to go speak at so this works well too 🙂
🔥 1
i'll probably be around this evening too if i can make enough progress on something else today