<@UB2J9BQA0> Do you have a handle on <https://gith...
# development
a
@hundreds-father-404 Do you have a handle on https://github.com/pantsbuild/pants/issues/8733 or looking for a hand?
w
To close the loop on this, I think that using ``UnionMembership`` as Danny was suggesting should work: it would look roughly like:
Copy code
sources = ..
for member in union_membership.members(Type):
  sources = await Get[FormattedSources](Type, member.cls(sources))
...ie, execute a series of formatting @rules in the order they were declared in the union. The order would be stable, but not customizable without more complexity... probably fine.
h
And will this need to be used for goals like test too, once we introduce junit?
w
No: see other response in the channel
👍 1
a
thanks for summarizing!!!
The order would be stable, but not customizable without more complexity... probably fine.
agreed! and if it does need to be customizable, i feel like that won't be super difficult to extend
@union
(?) to do
h
Trying to implement this this week. Check on my understanding:
TargetWithSources
is a union type and it’s fulfilled(?) by nested unions, e.g.
TargetWithSources
may be fulfilled by
FormattablePythonTarget
and
FormattableJavaTarget
Those are unions themselves. Then, tools define their own concrete dataclass like
IsortFormattableTarget
.
FormattablePythonTarget
would be fulfilled through union rules with
IsortFormattableTarget
,
BlackFormattableTarget
, etc
w
second paragraph looks right.
h
I’m possibly confused by this comment? https://github.com/pantsbuild/pants/issues/8343#issuecomment-559489622 I had a hard time following the conversation between what is proposed, should be done, and is only used by
--query
a
having unions within unions is not supported, but could be
the relevant part from
--query
is that you can use the
UnionMembership
singleton to query all members of a specific
@union
h
having unions within unions is not supported, but could be
Ah okay, that’s why this prototype fails. Would my summary even work for this problem? There’s still a path from
TargetWithSources->FormattablePythonTarget->IsortFormattableTarget
and also a path from
TargetWithSources->FormattablePythonTarget->BlackFormattableTarget
. There’s no way to disambiguate which of these two to use.
a
in
--query
, that’s used to kind of “forward declare” those union members, so that the union members can be overloaded to also be used to create instances of themselves with a parse method
looking at the code
h
looking at the code
let me post my WIP! https://github.com/pantsbuild/pants/pull/8794
a
basically you need to be requesting a specific path through the rule graph and in
--query
this is handled by another level of indirection
ok looking
w
@hundreds-father-404: the explanation for what I think should be done to allow for "multiple rules formatting one target" is in this thread in a comment
a
oh yeah
w
Eating lunch, so not sure whether the comment linked above is from before/after Danny and I talked about it and came up with the comment at the top here
h
This?
To close the loop on this, I think that using ``UnionMembership`` as Danny was suggesting should work: it would look roughly like:
sources = ..
for member in union_membership.members(Type):
sources = await Get[FormattedSources](Type, member.cls(sources))
...ie, execute a series of formatting @rules in the order they were declared in the union. The order would be stable, but not customizable without more complexity... probably fine. I didn’t follow
w
Yes
Which part?
h
The proposal still involves creating
IsortFormattableTarget
and
BlackFormattableTarget
, right? Are those directly members of the
TargetWithSources
union? There’s no intermediate type
PythonFormattableTarget
?
w
Type
in this example would probably be ... a union containing input types for various formatters... ie, ISortTarget and BlackTarget
You'd wrap some actual python target type in one of those wrappers temporarily to kick off formatting
(as in the example... it just doesn't fill in the union or the members)
h
Is this experiment on the right track? Each tool implements its own wrapper type like
BlackTarget
and
IsortTarget
and those are direct members of the union
rules.core.fmt.TargetWithSources
It fails with this message https://github.com/pantsbuild/pants/pull/8794#issuecomment-564769701, but maybe that’s resolvable by changing the implementation of the core
fmt
and
lint
rules as you suggested Stu?
w
this is a new union that is not "TargetWithSources"
h
this is a new union that is not “TargetWithSources”
Why is that / does
TargetWithSources
have any role still or that should be deleted?
w
and it would need to be python specific, because we would launch into the above loop based on this being "a python target'"
i don't know how
TargetWithSources
is used right now, but it's pretty abstract, and should likely be replaced with something as part of 4535
so, maybe
PythonFormattableTarget
... unknown: sorry. not familiar with where the unions are right now
h
Wait I’m confused. Are you envisioning that code snippet living in
rules/core/fmt.py
or somewhere else? I don’t think
fmt.py
can have any knowledge that the language Python even exists
w
this would need to be in the python backend somewhere, i think.
h
Right now, there’s
TargetWithSources
, which is used by both the core
fmt
and
lint
rules.
python/targets/formatabble_python_target.py
implements
FormattablePythonTarget
and registers that as a member of
TargetWithSources
. That file also registers the Python target adaptors as members of
TargetWithSources
and it registers rules going from each target adaptor to
FormattablePythonTarget
. Then, the Python linters ask for a
FormattablePythonTarget
in their rule signatures
w
"all targets with sources" are not necessarily formattable, so that formulation of TargetWithSources is confusing to me.
h
Agreed. It would be more clear to have
FormattableTarget
and then
PythonFormattableTarget
w
yea.
h
This was my first experiment. Go from
FormattableTarget -> PythonFormattableTarget -> BlackTarget|Flake8Target|IsortTarget
, but it failed because we can’t have nested unions
w
... need to push on 4535 to make the underlying classes (like
Target(v2)
) more useful so that not everything needs to be in unions
@hundreds-father-404: you could probably still do that with an intermediate
@rule
in the python backend. but yea.
h
Okay so is the second approach of
FormattableTarget -> BlackTarget|Flake8Target|IsortTarget
wrong? https://github.com/pantsbuild/pants/pull/8794
w
Yes
Because you don't want to run "all rules for all formattable targets" on each target
Or else you'll end up trying to run a Java formatter on python cofe
a
in the v2 export draft PR, i have a rule converting
TransitiveHydratedTargets
into
ThriftableTargetCollection
it gets around the fact that we can't do
HydratedTarget -> Optional[ThriftableTarget]
or something
w
Copy code
sources = ...
for member in union_membership.members(FormattablePythonTypes):
  sources = await Get[FormattedSources](FormattablePythonTypes, member.cls(sources))
h
Stu and I paired on this today and came up with this PR: https://github.com/pantsbuild/pants/pull/8801 It still fails, but we think is a bit closer to the solution. I posted the error in the PR. Any thoughts much appreciated on where to go from here!
a
Pushed a commit to the branch which ?maybe does the right thing?
💯 1
w
@hundreds-father-404: luck?
h
No, unfortunately. Daniel’s commit fixes the rule graph ambiguity, but then results in no targets ever running with
lint-v2
because none are satisfied with the
union_membership.is_member(LintTarget)
. Removing that check, then we get a runtime exception
Copy code
Exception: WithDeps(Inner(InnerEntry { params: {Specs, Console, OptionsBootstrapper}, rule: Task(Task { product: Lint, clause: [Select { product: Console }, Select { product: HydratedTargets }, Select { product: UnionMembership }], gets: [Get { product: LintResults, subject: LintPythonTarget }], func: lint(), cacheable: false, display_info: Some("lint-v2") }) })) did not declare a dependency on JustGet(Get { product: LintResults, subject: PythonTargetAdaptor })
I unfortunately won’t be able to finish the issue before leaving for Panama. I think Daniel may be right that we need to change the engine to allow unions within unions
w
Some wrapping is not happening there, that should be
That is a
Get
that is claiming a particular type, but not wrapping in it
I'll see if I can look at it tonight.
❤️ 1
@hundreds-father-404: just pushed a fix: https://github.com/pantsbuild/pants/pull/8801
h
😄 thanks
Didn’t realize you also fixed 8801! Thank you!!!
w
Sure thing!
h
8801 also allows us to finish decoupling
fmt
from
lint
! Yay!
w
@hundreds-father-404: two comments: the second might kill your run
👍 1