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

Update Jedi calls for its 0.17.0+ API #781

Merged
merged 24 commits into from
May 9, 2020
Merged

Update Jedi calls for its 0.17.0+ API #781

merged 24 commits into from
May 9, 2020

Conversation

bnavigator
Copy link
Contributor

@bnavigator bnavigator commented Apr 16, 2020

As discussed in #744 (comment), fixes #744, including most recent Jedi 0.17.0

This is a combination of #775 and dalthviz#1 Edit: removed the jedi master branch pulls in the CIs

Foxboron and others added 7 commits April 16, 2020 12:39
Signed-off-by: Morten Linderud <morten@linderud.pw>
fix path and project parameters for jedi_script
remove deprecated usages() calls
source is deprecated in 0.17.0 in favor of code
but code did not exist in 0.16.0
sometimes jedi reports some keywords first
Copy link
Contributor

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

@bnavigator, as part of this PR please also remove the code that tried to support Jedi 0.14 because it no longer applies (please look for JEDI_VERSION to do do that).

setup.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 changed the title Update JEDI calls for API >=0.16.0 Update Jedi calls for its 0.17.0+ API Apr 16, 2020
@ccordoba12
Copy link
Contributor

Thanks a lot @bnavigator for your help with this one! I'll review it tomorrow (I don't have time today, sorry).

@goanpeca
Copy link
Contributor

Thanks a lot for this @bnavigator!!

@goanpeca
Copy link
Contributor

IS this ready @ccordoba12 ?

@E1k3
Copy link

E1k3 commented Apr 29, 2020

Is there any update on this or is there any way to help finalizing this pull request?

@ccordoba12
Copy link
Contributor

IS this ready @ccordoba12 ?

I haven't had time to review it. Please help me with that and let me know if you need additional help.

Copy link

@E1k3 E1k3 left a comment

Choose a reason for hiding this comment

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

The changes seem good to me. The changes from davidhalter/jedi#1063 (comment) are accounted for and the new Project feature of jedi 0.17 is used.

I will now test this branch with vim + asyncomplete + vim-lsp for definition, hover, completion, references and signature. As far as I know, highlights are not implemented for this client, yet.

pyls/plugins/jedi_completion.py Outdated Show resolved Hide resolved
@E1k3
Copy link

E1k3 commented Apr 29, 2020

I have tested all mentioned features in a relatively simple script that imports various modules from Keras, NumPy, SciPy, Argparse and others and pyls worked flawlessly.

@ccordoba12 If more comprehensive testing or more detailed commentary on the changes are needed, please tell me in detail what you need..

@E1k3
Copy link

E1k3 commented May 3, 2020

@ccordoba12 Is there anything to be done with this request despite merging it?

The archlinux package of pyls is currently incompatible with jedi and workarounds are on hold, because this looks finished.

@bnavigator
Copy link
Contributor Author

bnavigator commented May 3, 2020

@E1k3, I recommend you apply the patch directly to the Archlinux package. That's what OpenSUSE is doing.

There may be some burnt ground between @ccordoba12 and me. Chances are, he will never accept this PR despite the clear demand from the community shown by the reactions in this thread and in #744.

@goanpeca
Copy link
Contributor

goanpeca commented May 4, 2020

@bnavigator we really appreciate your help with this work. We are a bit crowded right now and in the midst of making a release. Having this PR merged could delay our release or introduce some unwanted bugs with some other libraries introspection.

There may be some burnt ground between @ccordoba12 and me. Chances are, he will never accept this PR despite the clear demand from the community shown by the reactions in this thread and in #744.

We just need to review the PR (I have not found time yet!)

We also probably need to add some simple smoke tests with libraries that have broken with the past versions of Jedi (0.16).

Thanks again for the help!

@ccordoba12
Copy link
Contributor

Chances are, he will never accept this PR despite the clear demand from the community shown by the reactions in this thread and in #744.

This is not true. As @goanpeca said, we are swamped at the moment and haven't had time to review this PR, sorry. We'll try to do it and release 0.32.0 with these changes this week.

@ccordoba12
Copy link
Contributor

We also probably need to add some simple smoke tests with libraries that have broken with the past versions of Jedi (0.16).

I already added tests for Numpy, Pandas and Matplotlib. One of them detected an error in Jedi 0.16, which was fixed in 0.17

@bnavigator
Copy link
Contributor Author

Using _workspace.root_path didn't look like such a great idea.

>       project_path = self._workspace.root_path if self._workspace else os.path.dirname(self.path)
E       AttributeError: 'MockWorkspace' object has no attribute 'root_path'

@ccordoba12
Copy link
Contributor

Ok, on it. I think I know how to fix this.

@ccordoba12
Copy link
Contributor

I don't think your last commit is the right solution @bnavigator. I think the problem is in MockWorkspace.

Could you leave this alone for a few minutes? Thanks!

@bnavigator
Copy link
Contributor Author

If you allow Document(..., workspace=None) you cannot just assume that ._workspace is a workspace.

@ccordoba12
Copy link
Contributor

If you allow Document(..., workspace=None) you cannot just assume that ._workspace is a workspace.

I think it's better to use something that stands for a real Workspace object (I'm almost ready with my fixes).

@fsouza
Copy link
Contributor

fsouza commented May 9, 2020

@ccordoba12 I think what @bnavigator means is that now that workspace is required in Document, it shouldn't be an optional parameter in the constructor. As long as it's optional in the constructor, the code should support the case where it's None, even though in practice this never happens? 🤔

ccordoba12
ccordoba12 previously approved these changes May 9, 2020
Copy link
Contributor

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

I think this is finally ready. Thanks everyone (specially @bnavigator) for your patience.

@ccordoba12
Copy link
Contributor

You're totally @bnavigator and @fsouza! I'll do that last change here.

@ccordoba12 ccordoba12 dismissed their stale review May 9, 2020 20:58

Address last review

@bnavigator
Copy link
Contributor Author

bnavigator commented May 9, 2020

@ccordoba12 I think what @bnavigator means is that now that workspace is required in Document, it shouldn't be an optional parameter in the constructor. As long as it's optional in the constructor, the code should support the case where it's None, even though in practice this never happens?

Thanks for breaking it down @fsouza.

At least in the tests it happens all the time. And these are only the one-liners:

[ben@voyagerS9:~/src/python-language-server]% grep 'Document(.*)' * -R | grep -v workspace                                     [0]
test/fixtures.py:    return Document(DOC_URI, DOC)
test/plugins/test_autopep8_format.py:    doc = Document(DOC_URI, DOC)
test/plugins/test_autopep8_format.py:    doc = Document(DOC_URI, DOC)
test/plugins/test_autopep8_format.py:    doc = Document(DOC_URI, GOOD_DOC)
test/plugins/test_flake8_lint.py:    doc = Document(uris.from_fs_path(name))
test/plugins/test_flake8_lint.py:    doc = Document('', DOC)
test/plugins/test_folding.py:    doc = Document(DOC_URI, DOC)
test/plugins/test_folding.py:    doc = Document(DOC_URI, SYNTAX_ERR)
test/plugins/test_mccabe_lint.py:        doc = Document(DOC_URI, DOC)
test/plugins/test_mccabe_lint.py:    doc = Document(DOC_URI, DOC_SYNTAX_ERR)
test/plugins/test_pycodestyle_lint.py:    doc = Document(DOC_URI, DOC)
test/plugins/test_pydocstyle_lint.py:    doc = Document(DOC_URI, DOC)
test/plugins/test_pydocstyle_lint.py:    doc = Document(TEST_DOC_URI, "")
test/plugins/test_pydocstyle_lint.py:    doc = Document(DOC_URI, "")
test/plugins/test_pydocstyle_lint.py:    doc = Document(DOC_URI, "bad syntax")
test/plugins/test_pyflakes_lint.py:    doc = Document(DOC_URI, DOC)
test/plugins/test_pyflakes_lint.py:    doc = Document(DOC_URI, DOC_SYNTAX_ERR)
test/plugins/test_pyflakes_lint.py:    doc = Document(DOC_URI, DOC_UNDEFINED_NAME_ERR)
test/plugins/test_pyflakes_lint.py:    doc = Document(DOC_URI, DOC_ENCODING)
test/plugins/test_pylint_lint.py:        yield Document(uris.from_fs_path(name))
test/plugins/test_pylint_lint.py:        config, Document(uris.from_fs_path(__file__)), True)
test/plugins/test_pylint_lint.py:        config, Document(uris.from_fs_path(__file__)), False)
test/plugins/test_yapf_format.py:    doc = Document(DOC_URI, DOC)
test/plugins/test_yapf_format.py:    doc = Document(DOC_URI, DOC)
test/plugins/test_yapf_format.py:    doc = Document(DOC_URI, GOOD_DOC)
test/plugins/test_yapf_format.py:    doc = Document(uris.from_fs_path(src.strpath), DOC)
test/plugins/test_completion.py:    doc = Document(DOC_URI, DOC)
test/plugins/test_rope_rename.py:    doc = Document(DOC_URI)
test/test_document.py:    document_mem = Document(DOC_URI, u'my source')
test/test_document.py:    document_disk = Document(DOC_URI)
test/test_document.py:    doc = Document('file:///uri', u'')
test/test_document.py:    doc = Document('file:///uri', u'itshelloworld')
test/test_document.py:    doc = Document('file:///uri', u''.join(old))
test/test_document.py:    doc = Document('file:///uri', u''.join(old))

Copy link
Contributor

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

🎉

@ccordoba12 ccordoba12 added this to the 0.32.0 milestone May 9, 2020
@ccordoba12 ccordoba12 merged commit 2fb2c81 into palantir:develop May 9, 2020
@mattst88
Copy link

mattst88 commented May 9, 2020

Thanks a bunch to everyone involved!

If possible, please tag a release with this included. It'll help downstream Linux distros quite a lot.

@polyzen
Copy link

polyzen commented May 9, 2020

https://github.com/palantir/python-language-server/milestone/17

@Foxboron
Copy link
Contributor

Foxboron commented May 9, 2020

Thanks for the work getting this fixed :)

@ccordoba12
Copy link
Contributor

0.32.0 is out.

bmwiedemann added a commit to bmwiedemann/openSUSE that referenced this pull request May 29, 2020
https://build.opensuse.org/request/show/810112
by user scarabeus_iv + maxlin_factory
- Update to Version 0.33.0
  * Add optional class objects to completion list.
  * Fix completions with one arg.
  * Remove pycodestyle plugin's dependency on autopep8.
- Update to Version 0.32.0
  * Increase minimal supported version of flake8 and match
    flake8 requirements to PyLS ones.
  * Add support for renaming with Jedi.
  * Update Jedi calls for its 0.17 API
  * Fix flake8 io deadlock.
  * Follow LSP protocol when workspace folders are changed.
  * Adapt root workspace uri if folder is changed.
  * Add option to activate fuzzy completions in Jedi
- remove pach pyls-pr781-jedi-017.patch merged
  gh#palantir/python-language-server#781
- Bump requirements in spec file
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.

Adapt to the last Jedi
8 participants