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

Backtrack if incompatibilities fail to apply #65

Merged
merged 1 commit into from
Dec 27, 2020
Merged

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Dec 9, 2020

For pypa/pip#9180. The closure function looks so ugly but I can’t come up with something better :(

@uranusjr uranusjr changed the title Make py2index behaviour more obvious Backtrack if incompatibilities fail to apply Dec 9, 2020
@pradyunsg
Copy link
Contributor

I feel dumb -- I don't understand the need for the closure. :(

@pfmoore
Copy link
Collaborator

pfmoore commented Dec 14, 2020

I assumed it was just to name the block of code, for readability. If it's more than that, I'm as dumb as you, and I clicked "Approve" as well 🙂

@uranusjr
Copy link
Member Author

The closure is needed because we want to break a loop from a loop nested inside it. The high-level POV is

while len(self._states) >= 3:
    ...
    for k, incompatibilities in incompatibilities_from_broken:
        ...
        if criterion is None:
            # We want to break the "while" loop here.

And a closure with early return is the best way I can think of to do it.

@McSinyx
Copy link
Contributor

McSinyx commented Dec 15, 2020

I suppose we can use the following, but I'm not sure if it is any easier to read than a closure.

while ...:
    for ...:
        if ...:
            break
    else:
        continue
    break

@uranusjr
Copy link
Member Author

@McSinyx I thought of using else: as well, but preferred the closure approach. Also we need to continue the outer while, and I couldn’t find a nice way to do it with else:.

@pfmoore
Copy link
Collaborator

pfmoore commented Dec 15, 2020

I noticed that after I posted my comment. IMO using the closure for the early exit is fine, the code is perfectly readable as it stands.

@uranusjr uranusjr merged commit 164c427 into master Dec 27, 2020
@uranusjr uranusjr deleted the criteria-is-none branch December 27, 2020 12:14
@pradyunsg
Copy link
Contributor

I'm not sure if it is any easier to read than a closure.

FWIW, I would find this form easier to read, but only marginally, so 🤷. This does look good to me now that I'm looking at it with that context! ^>^

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.

4 participants