Fix some language server features not working in the console and in Jupyter notebooks e.g. hover and signature help #6140
Conversation
|
E2E Tests 🚀 |
|
Will debug the Windows CI issue once I've updated my Windows setup, but this is otherwise ready for review. |
There was a problem hiding this comment.
Since Jedi wasn't really designed to be customized at this level, patching lets us copy/paste a lot less of their code, which I think is more robust moving forward, and it's more succinct. Since we're patching a vendored version of Jedi, we also don't run the risk of messing with a user's own independent usage of Jedi.
This does look a lot cleaner! Moves like this remind me how nice it is we went the vendoring route 🫶
A few questions, but I played around with it a bit and it seems to work as intended!
|
|
||
| def _is_pandas_dataframe(value: Union[MixedObject, Value]) -> bool: | ||
| return ( | ||
| value.get_root_context().py__name__() == "pandas.core.frame" |
There was a problem hiding this comment.
pandas dataframes have changed names a handful of times; will this take into account the different names?
There was a problem hiding this comment.
I can't seem to find the code where we handled other import paths, maybe it became unnecessary at some point?
The tests in this PR pass with our minimum supported pandas (1.5.3) so I assume it's good to go.
| fname = oinspect.find_file(self._inspector.value) | ||
| if fname is not None: | ||
| # Normalize case for consistency in tests on Windows. | ||
| return Path(os.path.normcase(fname)) |
There was a problem hiding this comment.
I'm not entirely sure when module_path is used, but will this not give the wrong value on windows?
There was a problem hiding this comment.
I don't think so. os.path.normcase should never "break" a path e.g. if the file system is case insensitive it shouldn't change the case. Jedi seems to normcase almost everywhere as well.
| ) | ||
|
|
||
|
|
||
| def apply_jedi_patches(): |
There was a problem hiding this comment.
Nice to have this in its own function
The main goal of this PR is to address #5739. Along the way, I decided to refactor how we customize Jedi. I've moved away from subclassing to just patching things. Since Jedi wasn't really designed to be customized at this level, patching lets us copy/paste a lot less of their code, which I think is more robust moving forward, and it's more succinct. Since we're patching a vendored version of Jedi, we also don't run the risk of messing with a user's own independent usage of Jedi.
I also added a bunch of tests for different LSP features. I think it's worth having those since we might still want to run a variation of them even if we eventually move away from Jedi.
Release Notes
New Features
Bug Fixes
QA Notes
The linked issues have great repros. They should work in both notebooks and the console when you define an object in one execution, then trigger the LSP in another.