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

Rename repository modules #6803

Merged
merged 6 commits into from
Oct 31, 2022
Merged

Rename repository modules #6803

merged 6 commits into from
Oct 31, 2022

Conversation

b-kamphorst
Copy link
Contributor

@b-kamphorst b-kamphorst commented Oct 14, 2022

Relates-to: #3155

  • Added tests for changed code.
  • Updated documentation for changed code.

Follow-up of #6669 regarding the proposed renaming of modules in poetry/repositories.

A subsequent PR for poetry-plugin-export is in preparation: python-poetry/poetry-plugin-export#151.

@b-kamphorst
Copy link
Contributor Author

@neersighted let's make this fly. For now, I've added DeprecationWarnings that indicate support until poetry v1.4.0 (as to not be breaking for the next poetry minor release, 1.3.0). I noted that the deprecation warning for the poetry.core.semver.version:Version import did not state a version, but it might be good to do so.

You also mentioned the desire for a RepositoryName type that represents a normalized name. My (ugly) attempt at doing so broke the plugin and would be less ugly if rooted in poetry-core. Not sure if we want to pick this up, and if so, where.

Looking forward to your thoughts.

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

I propose to divide the rename and the deprecation into two commits as done in python-poetry/poetry-core#482. That way, it's easier to follow changes in git history and doing rebases of PRs changing code in one of the renamed modules.

@radoering
Copy link
Member

And it simplifies the review. 😉

@b-kamphorst
Copy link
Contributor Author

I propose to divide the rename and the deprecation into two commits as done in python-poetry/poetry-core#482. That way, it's easier to follow changes in git history and doing rebases of PRs changing code in one of the renamed modules.

Done!

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

Is there a reason for not having a deprecation warning for cached? (And for the exclamation mark in the commit message. refactor!: rename cached to cached_repository?)

As for the wording of the deprecation warning (if we want to add a "scheduled for removal notice" and if it will be 1.4), I'll leave that decision to @neersighted. Afaik we don't have a schedule yet for how long we keep deprecated code.

src/poetry/repositories/pool.py Outdated Show resolved Hide resolved
@b-kamphorst
Copy link
Contributor Author

Is there a reason for not having a deprecation warning for cached? (And for the exclamation mark in the commit message. refactor!: rename cached to cached_repository?)

Not really -- I hoped that this was used only internally and therefore we may not have to remain compatible. But that was just guessing so I'll change it (probably tomorrow). I guess in general with Python it is not quite clear what the API is -- harder to know what you can change without issues to others.

As for the wording of the deprecation warning (if we want to add a "scheduled for removal notice" and if it will be 1.4), I'll leave that decision to @neersighted. Afaik we don't have a schedule yet for how long we keep deprecated code.

If this is part of the public API, I'd expect deprecation in a major release. But I'm not sure how the poetry team applies versioning. We can leave it open but I think it is nice to at some point (timely) inform users when it is completely removed. Looking forward to your suggestions.

@b-kamphorst
Copy link
Contributor Author

@radoering, I've added the deprecation warning for poetry/repositories/cached. I think all that is left to finalize is the content of the warning message depending on your and @neersighted's thoughts?

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

We decided that the deprecation warnings are fine as they are. I just noticed a test that seems to be duplicated (see separate comment). Apart from that I think this PR is ready to merge.

tests/repositories/test_repository_pool.py Outdated Show resolved Hide resolved
@radoering radoering enabled auto-merge (rebase) October 31, 2022 08:30
@radoering radoering merged commit 6096ac5 into python-poetry:master Oct 31, 2022
@b-kamphorst b-kamphorst deleted the rename-repository-modules branch October 31, 2022 19:48
@neersighted neersighted added kind/refactor Pulls that refactor, or clean-up code impact/deprecation Introduces or relates to a deprecation area/sources Releated to package sources/indexes/repositories labels Nov 3, 2022
@neersighted neersighted added this to the 1.3 milestone Nov 3, 2022
Copy link

This pull request 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 Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/sources Releated to package sources/indexes/repositories impact/deprecation Introduces or relates to a deprecation kind/refactor Pulls that refactor, or clean-up code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants