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

Ensure repository deletion is consistent #6214

Merged
merged 2 commits into from
Aug 23, 2022
Merged

Ensure repository deletion is consistent #6214

merged 2 commits into from
Aug 23, 2022

Conversation

jre21
Copy link
Contributor

@jre21 jre21 commented Aug 22, 2022

This change improves the consistency of Pool().remove_repository() to make it easier to write poetry plugins which mutate the repository pool.

  1. Deleting an element from the middle of Pool()._repositories decrements the index of later entries. Update Pool()._lookup to reflect this.

  2. If a primary repository is deleted, decrement Pool()._secondary_start_idx to ensure that any additional primary repositories are still added before all secondary repositories.

  3. If the default repository is deleted, reset Pool()._default so a new one can be added.

Pull Request Check List

Resolves: #6213

  • Added tests for changed code.
  • Updated documentation for changed code. n/a: this change just updates an API to better match existing documentation

This change improves the consistency of Pool.remove_repository() to make
it easier to write poetry plugins which mutate the repository pool.

1.  Deleting an element from the middle of Pool()._repositories
decrements the index of later entries.  Update Pool()._lookup to reflect
this.

2.  If a primary repository is deleted, decrement
Pool()._secondary_start_idx to ensure that any additional primary
repositories are still added before all secondary repositories.

3.  If the default repository is deleted, reset Pool()._default so a new
one can be added.
@jre21
Copy link
Contributor Author

jre21 commented Aug 23, 2022

@neersighted Any chance this can get backported to the 1.2 branch, since it's a useful improvement to the new plugin functionality?

@neersighted
Copy link
Member

neersighted commented Aug 23, 2022

@neersighted Any chance this can get backported to the 1.2 branch, since it's a useful improvement to the new plugin functionality?

1.1 is only accepting backports for critical regressions, and future backports will be for major regressions only.

One could argue it's a correctness/soundness fix that deserves to make it into 1.2, but it's a higher risk change and I don't want to delay the 1.2 release unless @radoering and @dimbleby are confident in it.

Given 1.2 is reaching stabilization, this will likely make it in 1.3, unless we believe it is a blocker for 1.2

@jre21
Copy link
Contributor Author

jre21 commented Aug 23, 2022

As far as i can tell, poetry itself never calls remove_repository(), so this would just be a change that affects plugins -- and i would definitely make the correctness/soundness argument since the function's existing behavior leaves the pool in an inconsistent state in many circumstances.

@dimbleby
Copy link
Contributor

It's understandable that folk would ask for backports given the enormous amount of time that it has taken to release 1.2 - that sets expectations about when the next release will happen. But a much better thing would be for the project to shift to a mode where it released a lot more frequently.

As it goes I agree that poetry itself never even calls the method being changed, so it is unlikely to break anything, so "OK fine" is probably a safe answer for this one.

Still I would much prefer the answer to be "no, but we promise that the next release will follow shortly"...!

@neersighted
Copy link
Member

The intention is definitely for frequent minor releases with patch releases only for major regressions that make the previous minor unusable compared to its predecessor version. Please don't get scared of 'wait for 1.3' as that is intended to be short -- 1.2 was only heroic to release because of the multi-year gap and massive code churn.

I'm still open for arguments for inclusion in 1.2 even if I agree with @dimbleby and would prefer to see this in 1.3.

@radoering
Copy link
Member

Since it's a simple bugfix not adding new functionality, it's test coverage is quite good and it's not even used in poetry itself (lowest possible risk), I'd have no problem to accept a 1.2 backport after this PR is merged. However, I have no strong feelings about it.

Co-authored-by: Randy Döring <30527984+radoering@users.noreply.github.com>
@radoering radoering merged commit a66f99e into python-poetry:master Aug 23, 2022
neersighted pushed a commit to neersighted/poetry that referenced this pull request Aug 25, 2022
This change improves the consistency of `Pool().remove_repository()` to make it easier to write poetry plugins which mutate the repository pool.

1.  Deleting an element from the middle of `Pool()._repositories` decrements the index of later entries.  Update `Pool()._lookup` to reflect this.

2.  If a primary repository is deleted, decrement `Pool()._secondary_start_idx` to ensure that any additional primary repositories are still added before all secondary repositories.

3.  If the default repository is deleted, reset `Pool()._default` so a new one can be added.

Co-authored-by: Randy Döring <30527984+radoering@users.noreply.github.com>
neersighted pushed a commit that referenced this pull request Aug 25, 2022
This change improves the consistency of `Pool().remove_repository()` to make it easier to write poetry plugins which mutate the repository pool.

1.  Deleting an element from the middle of `Pool()._repositories` decrements the index of later entries.  Update `Pool()._lookup` to reflect this.

2.  If a primary repository is deleted, decrement `Pool()._secondary_start_idx` to ensure that any additional primary repositories are still added before all secondary repositories.

3.  If the default repository is deleted, reset `Pool()._default` so a new one can be added.

Co-authored-by: Randy Döring <30527984+radoering@users.noreply.github.com>
@jre21 jre21 deleted the remove_repository branch August 31, 2022 17:32
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve consistency of Pool().remove_repository()
4 participants