Skip to content
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

Test that classes without arguments contain parentheses in signature #182

Merged
merged 1 commit into from
Nov 22, 2021
Merged

Test that classes without arguments contain parentheses in signature #182

merged 1 commit into from
Nov 22, 2021

Conversation

ericvw
Copy link
Contributor

@ericvw ericvw commented Nov 19, 2021

Hey, @pappasam I noticed cfd2e7a and added a test case that I think covers the situation you were describing.

In my testing, it appears that .description() does include the parenthesis from Jedi. I hope you don't mind me reverting cfd2e7a towards keeping the code path simple now that there is a test in place. If there is a different situation that you are running into, I'm happy to iterate on the PR and ensure that case is covered.

Ensure consistency with function signatures that contain no arguments.
@ericvw ericvw marked this pull request as draft November 19, 2021 21:49
@ericvw
Copy link
Contributor Author

ericvw commented Nov 19, 2021

I just saw cfd2e7a#r60582938.

I've moved this as a draft until I understand what's happening in the specific example.

ericvw referenced this pull request Nov 19, 2021
This commit is helpful for consistent syntax highlighting for classes. A
current Jedi limitation is that Jedi will not return signatures for
classes with no arguments. For this reason, we explicitly append empty
parens in such cases for consistency.
@ericvw ericvw marked this pull request as ready for review November 19, 2021 21:58
@ericvw
Copy link
Contributor Author

ericvw commented Nov 19, 2021

@pappasam I wasn't able to reproduce what you described in cfd2e7a#r60582938 😕. FWIW, I use https://github.com/ericvw/dotfiles/blob/main/neovim/.config/nvim/lua/config/lsp.lua#L37-L47.

I'm happy to investigate further. If you prefer me to drop the revert and keep the test case, I can do that as well.

@pappasam
Copy link
Owner

pappasam commented Nov 22, 2021

@ericvw no worries, and I appreciate all your efforts! Like you, I like to keep the code as simple as possible.

Here's a screenshot of the hover operation using this PR with CodeAction in jedi_language_server.server:

image

Here's a screenshot of the same location from 8fe8340 (the current main):

image

Notice how, in the current main, there are trailing () after CodeAction while there are no trailing () after CodeAction in this PR? This is the edge case I'm referencing in cfd2e7a#r60582938. Since there are certain cases where the signature operation fails (I'm guessing it has something to do with c-extensions or something), I ensure consistency (eg, classes and functions at least get trailing parentheses) despite the fact that this isn't exactly correct either (eg, CodeAction should take arguments, but we sadly don't have access to this information from the latest version of jedi).

Note: I'm using coc.nvim with coc-jedi.

@ericvw
Copy link
Contributor Author

ericvw commented Nov 22, 2021

@ericvw no worries, and I appreciate all your efforts! Like you, I like to keep the code as simple as possible.

😄

Notice how, in the current main, there are trailing () after CodeAction while there are no trailing () after CodeAction in this PR?

I do see the difference in the two screenshots. I sense there is something deeper going on.

When I use the hover operation using this PR with CodeAction in jedi_language_server.server, this is what I get:

Screen Shot 2021-11-21 at 11 01 34 PM

Note: I'm using coc.nvim with coc-jedi.

If you don't mind keeping this PR open while, I would like to see if I can replicate what you are observing with coc.nvim and coc-jedi. I also noticed your dotfiles are at https://github.com/pappasam/dotfiles/tree/main/dotfiles/.config/nvim that should help as well.

To help me out, how do you set up your development for this project?

I roughly do the following:

$ git clone https://github.com/pappasam/jedi-language-server
$ cd jedi-language-server

$ git fetch origin pull/182/head:pr182
$ git checkout pr182

$ pyenv virtualenv 3.10.0 jedi-lsp-dev
$ pyenv local jedi-lsp-dev

$ make setup

$ nvim jedi-language-server/server.py
# Execute hover action.

I'll start looking into this more tomorrow and setting up coc.nvim coc-jedi.

@pappasam
Copy link
Owner

This tells me there might be a difference between our operating systems. I'm using Linux Mint 20.2; are you on an Ubuntu-based OS? Or something else? The difference may come down to Jedi's understanding of compiled C extensions, which will differ between operating systems.

@ericvw
Copy link
Contributor Author

ericvw commented Nov 22, 2021

@pappasam, you are correct that it is related to pydantic, and whether it is installed as pure Python or built as a Cython extension. Since I am using Python 3.0.0, I was getting the source distribution and a pure Python installation. When using Python 3.9, there is a pre-compiled Cython extension available as a wheel that I could replicate the issue with the built-in Neovim LSP client.

I'll keep the test case in this PR and drop the revert commit. I'll explore what is going on with Jedi and Cythoniozed pydantic independently.

@ericvw
Copy link
Contributor Author

ericvw commented Nov 22, 2021

PR has been updated with just the test case addition.

Copy link
Owner

@pappasam pappasam left a comment

Choose a reason for hiding this comment

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

lgtm!

@pappasam pappasam merged commit 98a297c into pappasam:main Nov 22, 2021
@ericvw ericvw deleted the ensure-class-parens branch November 22, 2021 19:08
@ericvw
Copy link
Contributor Author

ericvw commented Nov 22, 2021

I discovered davidhalter/jedi#1749 😢. It doesn't appear to support for pydantic.BaseModel in Jedi will be happening any time soon.

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.

2 participants