fix out-by-one in get_leaf_for_position

[dimbleby]

Fixes #217

I've taken the liberty of rewriting the binary search so that it's not recursive. To my tastes that's much easier to follow but I guess if you really liked it the way it was I can switch it back!

[dimbleby]

in fact let's not write our own binary search, there's one in the standard library.

Just need to make sure we use the right one out of bisect_left() vs bisect() / bisect_right()

[dimbleby]

ah no, the key on the standard library's bisect is new. So do write our own after all, sorry for the noise

[davidhalter]

Thanks for the patch! I will have to test with Jedi if all the tests still pass, but in general I feel like this is a good direction.

[dimbleby]

well this causes absolute carnage in jedi unit tests, I might need some help understanding why that is and whether this is salvageable...

[davidhalter]

Is there a way you can tell me what you changed without the refactoring? I have a hard time understanding that. I don't really understand the original issue either.

You don't have to revert all the changes, but maybe just post a few lines that you would change in the old code?

[dimbleby]

without the refactoring the change is just to the condition if position < element.end_pos, which originally was <=

the unit test is intended to show the problem: without the fix the element that is found is not the one at the given position.

[davidhalter]

This is intentional. It is unclear if the position is on the \n or on the next node. So this is kind of intentional, I guess. So I feel like we need to investigate why this has happened.

Jedi usually does this by first asking for get_name_of_position and only if no name is found it asks for get_leaf_for_position. So I guess the mistake is that we don't special case \n there. In that case (and if the next token is starting at the same point), we should be checking it there. So I'm inclined to close, because this should be done in Jedi. Do you agree? Do you want to work it out there?

[dimbleby]

I was coming to a similar conclusion: or at any rate that making a change along those lines was less likely to break the world

Round about https://github.com/davidhalter/jedi/blob/b814ca2951b29be2a103395039240dbe6c5c154d/jedi/api/__init__.py#L371, something like this, perhaps?

+
+        if leaf is not None and leaf.end_pos == (line, column) and leaf.type == 'newline':
+            next_ = leaf.get_next_leaf()
+            if next_ is not None and next_.start_pos == leaf.end_pos:
+                leaf = next_
+

[dimbleby]

davidhalter/jedi#1923