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.

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

bring back set_version (or similar) to restore ability to dynamically set package version in plugins #6223

Closed
2 tasks done
bcavagnolo opened this issue Aug 23, 2022 · 9 comments · Fixed by python-poetry/poetry-core#447
Labels
kind/feature Feature requests/implementations

Comments

@bcavagnolo
Copy link

  • I have searched the issues of this repo and believe that this is not a duplicate.
  • I have searched the documentation and believe that my question is not covered.

Feature Request

Hello. I am current trying to move from poetry 1.2.0a2 to 1.2.0rc1. I'm maintaining a custom version plugin that relies on the set_version function which was removed in this commit: a5ab9ce.

I'm struggling to find an alternative for this function. I noticed this idea:
#144 (comment)

...but it alters the pyproject.toml file, which dirties up my version control story.

I headed over to discord to discuss it there. It sounds like a PR to add this feature back would be welcome.

@bcavagnolo bcavagnolo added kind/feature Feature requests/implementations status/triage This issue needs to be triaged labels Aug 23, 2022
@radoering
Copy link
Member

Please consider that the version of a Package must be immutable because it's used in its hash method. There has already been a discussion about it in python-poetry/poetry-core#330

I only see two options at the moment:

  1. Make the version of ProjectPackage (which inherits from Package) mutable and consequently make ProjectPackage unhashable.
  2. Implement a with_version() method that basically returns a copy of the package with another version. Here is an example of a similar implementation.

The first option carries a certain risk since we might use ProjectPackage in places where we should use Package. See python-poetry/poetry-core#330 (comment). However, I have the hope that tests would reveal that.

The second option is not exactly setting the version, so you have to check if it even covers your use case.

@dimbleby
Copy link
Contributor

previously #5486 (comment) (but presumably the plugin author in that conversation didn't care enough to submit an MR)

@bcavagnolo
Copy link
Author

@radoering thanks for the feedback. I see your point about the mutability of versions and hashability of the Package type. I'm essentially trying to bring back the version plugin example in the 1.2.0a1 blog post. My concern about approach 2 is that in the plugin context the ProjectPackage comes indirectly from poetry.package. If we pursue this approach, would the plugin assign the package? Something like poetry.package = poetry.package.with_version(...)?

FWIW, in my PR (python-poetry/poetry-core#447) I tried approach 1. The poetry-core tests are satisfied. But many upstream poetry tests fail with the unhashable exception I'm raising: https://github.com/python-poetry/poetry-core/runs/7998228448?check_suite_focus=true.

@radoering
Copy link
Member

radoering commented Aug 24, 2022

Yeah, in the downstream tests I see that approach 1 can't succeed. We don't use ProjectPackage instead of Package accidentally, it just has to be hashable.

I suppose setting poetry.package is not allowed at the moment - and I'm not sure if that's a good idea.

There might be one more option considering the hash method: We could overwrite __hash__ in ProjectPackage so that it doesn't use the version. As long as __eq__ still considers the version everything should be fine. Not using it in __hash__ should only influence performance of dict/set lookups but since we normally don't have several ProjectPackages with different versions that shouldn't matter.

@bcavagnolo
Copy link
Author

Okay I updated my PR. The it makes the ProjectPackage use the grandparent __hash__. The existing parent __eq__ checks the version. This implementation makes assumptions about the inheritance. Is this acceptable? In any case, the downstream tests passed.

@radoering
Copy link
Member

This implementation makes assumptions about the inheritance. Is this acceptable?

IMO it's acceptable, at least I can't think of a better solution for now. I added a test that checks that the hash does not change when changing the version so we should notice if we break __hash__ by adding an additional level of inheritance or similar.

@radoering
Copy link
Member

Resolved by python-poetry/poetry-core#447

@bcavagnolo
Copy link
Author

Thank you @radoering! I'm grateful that our 1.2 migration path is preserved!

Copy link

github-actions bot commented Mar 1, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Feature requests/implementations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants