Fix dropped markers in dependency walk #4686
Conversation
|
While I'm here: among the code that I was confused by in this method was poetry/poetry/packages/locker.py Lines 263 to 265 in c6b19b3
I tried removing this block altogether and no tests failed, I suspect that it may not be wanted. |
There was a problem hiding this comment.
Thanks for the excellent work here -- this is a very minimal and easy to understand changeset. Thanks @johnmacnamararseg for the excellent test case as well.
|
Shoot, I forgot to add a Co-authored-by line to that commit, as the test was from #3512. Sorry @johnmacnamararseg. |
A lack of a rebase before merge kept this intersection of changes from being properly tested.
…ry#4753) A lack of a rebase before merge kept this intersection of changes from being properly tested.
|
Any updates on a release that will include this fix? |
…ry#4753) A lack of a rebase before merge kept this intersection of changes from being properly tested.
This is a backport of python-poetry#4686. Note that the test is changed due to a regression in `master` -- somewhere, the ability to drop two markers that negate each other was lost. Co-authored-by: Bjorn Neergaard <bjorn@neersighted.com>
This is a backport of #4686. Note that the test is changed due to a regression in `master` -- somewhere, the ability to drop two markers that negate each other was lost. Co-authored-by: Bjorn Neergaard <bjorn@neersighted.com>
|
@dimbleby thanks for the improvement and helping get this across the finish line. much easier to follow than what I originally had. very happy to see this merged in! |
|
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. |
Pull Request Check List
Resolves: #3511
I would very much like a fix for #3511, and tonight for the first time I tried to understand the proposed fix at #3512. I must admit I found it all a bit confusing! Here is a version that makes a much more localised change - my hope is that this is more palatable for merging.
Again the aim is to make sure that when we encounter an already-encountered dependency, we do not drop the associated markers. But I've done this just by moving where we make the
if key not in nested_dependenciescheck.It's definitely possible that I've missed some good reason why #3512 is more complicated than this; but I have borrowed the unit test case added at #3512 and that still passes (and fails without the fix). And I've also tested the fix in a couple of real life projects where I have run into this bug, and it does the job for those too.
@johnmacnamararseg @abn