hundreds-father-404
03/21/2020, 3:57 PMAddressWithOrigin
and TargetWithOrigin
. We’re using dataclasses to represent this.
I’d like to instead use typing.NamedTuple
, which has the exact same class
definition as a dataclass, including allowing us to define methods, but is more performant and allows us to unpack the pair:
target, origin = target_with_origin
But, the one downside is that TargetWithOrigin(tgt, origin) == (tgt, origin) == DistinctNamedTuple(tgt, origin)
. (TargetWithOrigin != tuple != DistinctNamedTuple
, at least).
I don’t think this is actually an issue? We don’t care if TargetWithOrigin(tgt, origin) == (tgt, origin)
occurs and because the engine calls type()
on instances to use exact type IDs, we would fail if a rule author forgot to wrap the tuple in its corresponding NamedTuple?aloof-angle-91616
03/21/2020, 3:59 PMhundreds-father-404
03/21/2020, 4:00 PMi would really prefer if the class name was required to destructure,I don’t think we generally want that for complex types: see https://www.python.org/dev/peps/pep-0557/#why-not-just-use-namedtuple for why this makes
XWithY
, we should never be adding a 3rd field so it would be super nice to have.aloof-angle-91616
03/21/2020, 4:00 PMInstances are always iterable, which can make it difficult to add fields.i don’t like this part
hundreds-father-404
03/21/2020, 4:04 PMNamedTuple
means it’s much more difficult to add a new field to the class than before because you break all call sites that now need to unwrap that new fieldTarget
and StripTargetRequest
. One nice feature is having custom constructors that can be frozen after (thanks again for that decorator!). Where we can optimize is the pair types we have like AddressWithorigin
aloof-angle-91616
03/21/2020, 4:07 PM..
), then this seems like it’d be a lot more usefulhundreds-father-404
03/21/2020, 4:09 PMi don’t know how i feel about that.Feel about what? About using
NamedTuple
with pair types like AddressWithOrigin
? They can keep using named properties like address_with_origin.address
if they want! No need to rewrite those call sites. Only, now, there’s an additional technique they can use to destructure via tuple unpackingaloof-angle-91616
03/21/2020, 4:10 PMwitty-crayon-22786
03/21/2020, 10:55 PMaloof-angle-91616
03/21/2020, 10:56 PMwitty-crayon-22786
03/21/2020, 10:56 PMhundreds-father-404
03/21/2020, 11:00 PMTargetsWithOrigins
and AddressesWithOrigins
. What would that look like with multiple params?witty-crayon-22786
03/21/2020, 11:07 PMhundreds-father-404
03/21/2020, 11:16 PMAddressWithOrigin
via multiple params.
`NamedTuple`s are exceptionally lightweight (Py 3.9 made an optimization where their attribute access is the fastest in Python, apparently). It’s also a very nice construct for programmers. They can easily wrap their head around having “both an Address and an OriginSpec” - it’s a plain and simple Python idiom with no new engine magic.
Another consideration. For AddressesWithOrigins
and TargetsWithOrigins
, we must preserve the method .expect_single()
.witty-crayon-22786
03/21/2020, 11:22 PMAddresses
, Targets
, and Origins
hundreds-father-404
03/21/2020, 11:25 PMOriginSpec
at the time of first parsing the Addresses
. We must preserve that information when we first encounter it or we will have no way of backtracking “What command line spec did :lib
come from..?”
AddressesWithOrigins
is the root of it all. From there, we can get Addresses
by stripping off the origins, for example; or convert it to TargetsWithOrigins
witty-crayon-22786
03/21/2020, 11:25 PMhundreds-father-404
03/21/2020, 11:27 PMwhere origins was a dict , and you could look up the origin for an Address or Target via addressWe probably could do that..but I don’t see the benefit? There is a tight coupling between an
Address
and its OriginSpec
. Every Address
has precisely one OriginSpec
.
Right now, we preserve that information when we first resolve the addresses. Throwing it away and replacing it with this Origins
singleton could work, but seems round-aboutwitty-crayon-22786
03/21/2020, 11:27 PMTargetsWithOrigins
, in particular.hundreds-father-404
03/21/2020, 11:28 PMit involves fewer classes that are just compositions of other classesAt the expense of more magical singletons and rules needing to request both
Targets
and Origins
, rather than simply TargetsWithOrigins
witty-crayon-22786
03/21/2020, 11:28 PMAddresses
already exists.hundreds-father-404
03/21/2020, 11:29 PMOrigins
typewitty-crayon-22786
03/21/2020, 11:29 PMhundreds-father-404
03/21/2020, 11:32 PMin the one merged parameter case you need a wrapper classKeep in mind that if you don’t care about
WithOrigins
, you can still request the simpler Addresses
. You only request AddressesWithOrigins
when you have something meaningful to do with the origin specs.witty-crayon-22786
03/21/2020, 11:33 PMhundreds-father-404
03/21/2020, 11:34 PMlint
, fmt
, and test
so that we can calculate the precise files to run on, e.g. ./v2 test foo.py
.
No other goals use it and simply request `Addresses`/`Targets`witty-crayon-22786
03/21/2020, 11:34 PMhundreds-father-404
03/21/2020, 11:35 PMor only in error cases?We never use the
OriginSpec
for errors, at the moment. This feature is solely used for precise file arguments, which is something rule authors have to go out of their way to opt intowitty-crayon-22786
03/21/2020, 11:36 PMdataclass
: datatype
used to extend namedtuple
aloof-angle-91616
03/21/2020, 11:37 PMwitty-crayon-22786
03/21/2020, 11:37 PMhundreds-father-404
03/21/2020, 11:37 PMmy understanding when we moved to dataclass was that it was similarly performant to namedtupleYes, dataclass still has very good and optimized performance. But NamedTuple is even more lightweight because it’s doing less things than a fully fledged class
witty-crayon-22786
03/21/2020, 11:38 PMhundreds-father-404
03/21/2020, 11:39 PM__slots__
to reduce a class’s memory footprint. Dataclasses don’t do that by default (they considered it) because it makes things much more complex. In contrast, because NamedTuple is just a wrapper around a tuple, it (I believe) uses that performance hackwitty-crayon-22786
03/21/2020, 11:40 PMComparison
tablehundreds-father-404
03/21/2020, 11:41 PMfrozen=True
, which is known to slightly reduce performance https://docs.python.org/3/library/dataclasses.html#frozen-instancesNamedTuple
for TargetWithOrigin
and AddressWithOrigin
. I suspected it would be better so threw it out there as an additional carrot
What I really want is tuple unpacking:
tgt, origin = target_with_origin
witty-crayon-22786
03/24/2020, 1:46 AMAddresses
via AddressesWithOrigins
causes about a 10-15% slowdown for list
in 1.26.x
hundreds-father-404
03/24/2020, 2:13 AMwitty-crayon-22786
03/24/2020, 2:27 AMhundreds-father-404
03/24/2020, 2:28 AM