rename now reports correct number of lines.#345
Conversation
It was initially reporting `sys.maxsize` all of the time, and this was both incorrect and problematic for some clients.
|
Thanks for your interest in palantir/python-language-server, @abingham! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
| @@ -1,4 +1,6 @@ | |||
| # Copyright 2017 Palantir Technologies, Inc. | |||
| from functools import reduce | |||
There was a problem hiding this comment.
Can you just import functools? I (and probably the linter!) don’t like overriding the built-in function.
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _num_lines(resource): |
There was a problem hiding this comment.
Can you move “private” functions to bottom of module
|
|
||
| def _num_lines(resource): | ||
| """Count the number of lines in a `File` resource. | ||
| """ |
There was a problem hiding this comment.
Closing quotes on the same line if they fit
| def _num_lines(resource): | ||
| """Count the number of lines in a `File` resource. | ||
| """ | ||
| s = io.StringIO(resource.read()) |
There was a problem hiding this comment.
What does resource.read() return? Seems unnecessary to use reduce given you’re loading the resource into memory anyway. Can’t you just call splitlines and count it?
There was a problem hiding this comment.
Yeah, the reduce call was a bit too cutesy.
|
I think I've made all the changes you asked for. I also added |
| 'uri': uris.uri_with( | ||
| document.uri, path=os.path.join(workspace.root_path, change.resource.path) | ||
| ), | ||
| 'version': document.version |
There was a problem hiding this comment.
This is incorrect, the uri we're returning is derived from the rope resource (line above), however this version number is derived from the document within which the rename was triggered.
If you check the LSP, version is not a required field: https://microsoft.github.io/language-server-protocol/specification#versionedtextdocumentidentifier
However, we can do better than just never setting it. If you use workspace.get_document(doc_uri) and then use that document's version then it will return None (correctly) if the server has never received a didOpen message, or the correct version number if it has.
My concern with doing this is that rope doesn't operate on the in-memory view of the documents. We should probably fix that before making too heavy usage of rope. In particular, the edits returned by rope could be inconsistent with the version of the document.
There was a problem hiding this comment.
So is it incorrect that the client is requiring the version? I think they're only accidentally requiring it, but I don't know for sure.
Regarding rope: yes, it would be nice if we could get it to use the in-memory buffer. In my earlier work with rope I just had the client save the file, but that was the lazy way out.
There was a problem hiding this comment.
Yup, I think it's incorrect of the client to require it. Though it might be correct if they only require it for documents that the client has sent a didOpen message for
This based on discussion in PR palantir#345.
|
The |
It was initially reporting
sys.maxsizeall of the time, and this was bothincorrect and problematic for some clients.
See #218 for more information.