sourcegraph / python-langserver

Language server which talks LSP via JSONRPC for Python.
MIT License
102 stars 12 forks source link

Add simple tests and adjust our workspace internal module resoultion logic #39

Closed ggilmore closed 6 years ago

ggilmore commented 6 years ago

This PR includes a simple test project with no third party dependencies, along with a corresponding test suite. It's not exhaustive (since there are no external dependencies), but having test cases that don't break due to not having pinned versions of external packages is valuable.


In the course of writing these tests, I discovered an issue when we try to jump to def / hover over this line: https://github.com/sourcegraph/python-langserver/blob/simple-tests/test/repos/fizzbuzz_service/fizzbuzz_service/checkers/fizzbuzz/fizzbuzz_checker.py#L6:25 (No definition would be returned).

After tracing our modifications to jedi, I realized that our logic to search for internal project packages seems incorrect.

The relevant directory structure for the file mentioned above is:

/fizzbuzz_service
/fizzbuzz_service/checkers
/fizzbuzz_service/checkers/fizz
/fizzbuzz_service/checkers/fizzbuzz
/fizzbuzz_service/checkers/fizzbuzz/fizzbuzz_checker.py

fizzbuzz_checker.py has a relative import that the top of the file for ..fizz. The .. refers to the parent checkers package.

Previously, when resolving the checkers package (and weren't given any specific paths to check for checkers from jedi) - we'd only consider the paths:

when looking for the checkers package. None of the paths above are correct (and two don't exist). We'd then assume that checkers is some external pypi package. checkers isn't a pypi package, so fetching that would fail and cause the response to be incorrect.

The fix is to more closely mimic jedi's strategy when resolving these imports. When jedi is asked to resolve the checkers import on /fizzbuzz_service/checkers/fizzbuzz/fizzbuzz_checker.py, it:

My solution mimics this behavior, and includes a test case for the above example.

ggilmore commented 6 years ago

I'd appreciate someone to double check the logic behind my fix, and to make sure that it's correct behavior and mimics what python/jedi actually does.

sqs commented 6 years ago

I'd appreciate someone to double check the logic behind my fix, and to make sure that it's correct behavior and mimics what python/jedi actually does.

Reviewing now