Skip to content

Merge bisect #468

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

Merged
merged 2 commits into from
Aug 24, 2016
Merged

Merge bisect #468

merged 2 commits into from
Aug 24, 2016

Conversation

tharvik
Copy link
Contributor

@tharvik tharvik commented Aug 11, 2016


def bisect_left(a: Sequence[_T], x: _T, lo: int = ..., hi: int = ...): pass
def bisect_right(a: Sequence[_T], x: _T, lo: int = ..., hi: int = ...): pass
def bisect_left(a: Sequence[_T], x: _T, lo: int = ..., hi: int = ...) -> int: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

It's perfectly legal for x to not be in a.

For example,

bisect.bisect_left([1,2,3], None)

is valid code. (And bisect_left will just return 0 in this case)

So the type parameter here is overkill.

@gvanrossum
Copy link
Member

That seems a 2/3 difference though -- in PY3 None < 1 is a TypeError.

@matthiaskramm
Copy link
Contributor

But even Python 3 allows

bisect.bisect_left([1, 2, 3], 5.391)

which is still a type error for

def bisect_left(a: Sequence[_T], x: _T)

@gvanrossum
Copy link
Member

But do you have code that relies on that? Usually a bisect() call is followed by an insert() at the found position, and inserting 5.391 into a List[int] is a type error. So I still prefer the PR as-is here rather than the original object.

@matthiaskramm
Copy link
Contributor

test_bisect.py itself is doing bisect_left([1, 2, 3], 1.5) for testing.

In pysco, I found the little gem

i = bisect.bisect(codeboundary, (self.addr-0.5, self))`

In our codebase, I'm also seeing a few occurences of the pattern bisect(seq, (low + high) * 0.5), presumably for getting some kind of percentiles.

I know it's tempting to use typeshed to define APIs the way we think they should be used, rather than how people are currently using them. But let's resist that and model the real world.

@JukkaL
Copy link
Contributor

JukkaL commented Aug 17, 2016

Hmm I think that bisect.bisect_left([1, 2, 3], 5.391) should be okay even if the definition is like this:

def bisect_left(a: Sequence[_T], x: _T) -> int: ...

Since int gets promoted to float, we'd infer _T = float. And as sequence is covariant, List[int] is compatible with Sequence[float].

@matthiaskramm
Copy link
Contributor

Tried your suggestion in mypy, but got:

Cannot infer type argument 1 of "bisect_left"

@JukkaL
Copy link
Contributor

JukkaL commented Aug 18, 2016

That looks like a mypy bug to me (filed python/mypy#2035).

@gvanrossum
Copy link
Member

The mypy bug won't be fixed soon (it's a bit tricky). Can you either put a # type: ignore on the line or use Any for the type of x?

@tharvik
Copy link
Contributor Author

tharvik commented Aug 24, 2016

Gone for a TODO and using Any.

@matthiaskramm matthiaskramm merged commit 9f1e90b into python:master Aug 24, 2016
@tharvik tharvik deleted the merge_bisect branch August 25, 2016 10:00
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