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

start_backtracking suggestion #81

Merged
merged 10 commits into from
Oct 11, 2021

Conversation

nadavwe
Copy link
Contributor

@nadavwe nadavwe commented Jul 29, 2021

resolvelib changes to solve pypa/pip#10210.

Comment on lines +143 to +145
backtracking_causes = run_resolver([("a", {1, 2}), ("b", {1})])
exception_causes = run_resolver([("a", {2}), ("b", {1})])
assert exception_causes == backtracking_causes
Copy link
Contributor Author

@nadavwe nadavwe Jul 29, 2021

Choose a reason for hiding this comment

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

I wanted to make it obvious that the backtracking causes are exactly the same as the failure when the "best" version is selected.
@uranusjr do you prefer I give an explicit value instead?

Copy link
Member

Choose a reason for hiding this comment

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

This is better IMO if you want to express the causes should be the same.

@uranusjr
Copy link
Member

I’m +1 to the idea, but we should use a different function name. It’s not at all clear what the difference between start_backtracking and backtracking is (also the tense is inconsistent to other functions).

Maybe we can call this something like resolving_conflicts?

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Some minor changes are needed.

src/resolvelib/reporters.py Outdated Show resolved Hide resolved
src/resolvelib/resolvers.py Outdated Show resolved Hide resolved
nadavwe and others added 2 commits October 3, 2021 10:31
@nadavwe nadavwe requested a review from uranusjr October 3, 2021 07:32
@uranusjr
Copy link
Member

uranusjr commented Oct 6, 2021

Uh, Travis is dead somehow. We should migrate to GitHub Action.

@uranusjr
Copy link
Member

uranusjr commented Oct 6, 2021

Could you rebase to or merge the latest master branch so we can apply the new GitHub Action checks?

@uranusjr
Copy link
Member

uranusjr commented Oct 7, 2021

Linter error!

@nadavwe
Copy link
Contributor Author

nadavwe commented Oct 7, 2021

Linter error!

sorry for that... :/

@uranusjr
Copy link
Member

uranusjr commented Oct 8, 2021

Nothing to be sorry about, the purpose of the CI being there to catch our errors 😛

@uranusjr
Copy link
Member

uranusjr commented Oct 8, 2021

Now test failures

@nadavwe
Copy link
Contributor Author

nadavwe commented Oct 10, 2021

Nothing to be sorry about, the purpose of the CI being there to catch our errors 😛

If I had run nox properly before committing, none of those problems would have occurred...
that's why I'm sorry, needless ping-pongs. 🏓 😳

anyway, I ran nox now and it passes locally, I hope that CI agrees with me. 🤞

@uranusjr uranusjr merged commit 12cfc09 into sarugaku:master Oct 11, 2021
@nadavwe
Copy link
Contributor Author

nadavwe commented Oct 11, 2021

@uranusjr Thanks for the merge!
when do you think resolvelib would be updated in pip so I can continue the integration in pypa/pip#10258?
or maybe I can do this update on my own and open a PR to pip?

@uranusjr
Copy link
Member

Either would do, but first we'll need a new release here since pip's vendoring mechanism only allows public release. Since this does not introduce any backward incompatibility, I'll find some time to do a release this week.

@uranusjr
Copy link
Member

0.8.1 released with this change.

@nadavwe
Copy link
Contributor Author

nadavwe commented Oct 19, 2021

opened pypa/pip#10600 to incorporate the new version into pip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants