Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Dependency.set_extras() #373

Closed
wants to merge 1 commit into from
Closed

add Dependency.set_extras() #373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/poetry/core/packages/dependency.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attribute change can be cleanly split out of #370. I can do it in another PR if you do not want to add it to this one, easy enough.

We can handle with_constraint() implementation in another PR as the need arises or once we settle #370.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: attribute removal #375

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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__()).

self._extras = frozenset(extras)

@property
def in_extras(self) -> list[str]:
return self._in_extras
Expand Down
7 changes: 7 additions & 0 deletions tests/packages/test_dependency.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,13 @@ def test_dependency_string_representation(
assert str(dependency) == expected


def test_set_extras_sets_extras() -> None:
dependency = Dependency("A", "^1.0")
assert not dependency.extras
dependency.set_extras(["foo", "bar"])
assert dependency.extras == frozenset(["foo", "bar"])


def test_set_constraint_sets_pretty_constraint() -> None:
dependency = Dependency("A", "^1.0")
assert dependency.pretty_constraint == "^1.0"
Expand Down