add Dependency.set_extras() #373
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
| @@ -244,6 +244,9 @@ def transitive_python_constraint(self) -> VersionConstraint: | |||
| def extras(self) -> frozenset[str]: | |||
| return self._extras | |||
|
|
|||
| def set_extras(self, extras: Iterable[str]) -> None: | |||
There was a problem hiding this comment.
I wonder if it would be better to follow the with_extras() pattern similar to package.with_features(). The whole extras / features thing is a bit confusing at present.
Because you are doing this anyway, I would suggest you roll in the removal of the Dependency._extra attribute in favour of the PacakgeSpecification._feature change from https://github.com/python-poetry/poetry-core/pull/370/files#diff-93744eaedc27b829eefbde40b5d45732da68e0716af35c30bf010e66ea72b1f7R84-R245 here too.
There was a problem hiding this comment.
I wondered about with_extras(), but then we ought to do with_constraint() too and it all seemed like a lot of trouble for no very clear benefit.
#370 looks a bit hairy, happy to wait for the dust to settle on it before pushing forward on this one but I don't think I want to get into doing both at once...
There was a problem hiding this comment.
We should really be careful that we don't tear down right away what will be cleaned up in #370. If we decide to use an attribute in the hash, it should not be changed anywhere. Thus, the with_attribute() pattern is required.
There was a problem hiding this comment.
You say required, but we already don't follow that in set_constraint(), and the single place that calls that (and would call this new setter) is safe.
So I only sort of agree...
There was a problem hiding this comment.
"Required" for clean code I assume.
That we don't follow the pattern somewhere does not mean that this is alright. Of course, we could allow setting attributes that are used in hash and evaluate each place that sets the attribute. But you don't have to be a prophet to see that eventually someone will forget that it's dangerous to set this attribute... Thus, in my opinion it may be better not to provide a setter but just a with_attribute() (or don't use the attribute in __hash__()).
anticipating that <python-poetry/poetry-core#373> will remove _extras
|
Actually I might be changing my mind about the best way to fix this... The current approach is vulnerable to I previously rejected a simpler fix that resulted in an export like: but actually while that's cosmetically awkward, it seems like it should give the right answers and be more reliably right. Rework coming, and in that case #373 will be redundant anyway |
|
That resembles the original fix I was playing with that led me down the hash rabbit hole. But yeah I think that is more accurate anyway. It is upto whoevever consumes the export to decide how to handle it I reckon. |
Will allow tidying of python-poetry/poetry#5688, which currently is manipulating
_extrasdirectly.Again the split into multiple repositories makes it complicated to figure out a merge sequence that will work...