-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Patch Jedi to get completions for compiled objects in Numpy and Matplotlib #3808
Conversation
@@ -0,0 +1,286 @@ | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this file to the spyder/utils/introspection
folder
def _search_return_in_numpydocstr(docstr): | ||
r""" | ||
Search `docstr` (in numpydoc format) for type(-s) of `param_str`. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this blank line
False | ||
>>> _search_param_in_docstr(':param int param: some description', 'param') | ||
['int'] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this blank line
|
||
See also: | ||
http://sphinx-doc.org/domains.html#cross-referencing-python-objects | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this blank line
Thanks so much for working on this! I left some minor style comments for now :-) To fix our tests on CircleCI you need to rebase your work on top of the latest 3.x branch. Besides, it'd be nice if you could add some tests for this new addition and also move our tests at the end of https://github.com/spyder-ide/spyder/pull/3715/files#diff-4e3108d8800283797868af8e23210200 |
@ccordoba12 I can make all these changes, but I think I need to make slightly larger changes for the compiled objects returns. I'll patch Jedi like rope is currently with a function in a separate file (like jedi_patch.py) so I can patch some Jedi methods. I can also set a limit on the patch to Jedi == 0.9.0 because I hope all this will be included for the 0.10.0 release. Should I make these changes in this pull request or make a new one? I think I can do it here without making a mess. |
Sure, please make them here :-) |
Numpy completes in Spyder with this commit but I still need to finish the testing and something else is wrong in matplotlib. Testing is incomplete and breaking stuff. I just wanted to push to test it at work. @ccordoba12 Where do I see the output of the debug_print() functions in jedi_plugin.py? I'm currently testing spyder with "bootstrap.py --debug" |
In the terminal where you started Spyder, but for that you need to start Spyder like this
(assuming your using our |
@ccordoba12 I've had some good progress with this. I now have mpl Axes objects returning without modifying matplotlib. I edited the first comment to show what works now. I added a numpydoc dependancy to try to fix the CircleCl test but it still doesn't pass. I think I'm ready for review. |
Thanks @goanpeca. I'll add numpydoc to the circle.yml Since both numpy and matplotlib are optional, I'm going to change numpydoc to an optional dependency as well. I'll change the test_jedi_plugin to make the numpy and matplotlib tests skippable using pytest.skipif if they are not installed |
TODO: Testing is incomplete
FIX: matplotlib Axes completions ADD: numpydoc dependency
FIX: added numpydocs and matplotlib to circle.yml
I have to say you've done a terrific job here, congrats! Your tests are all passing in Python 3.5, but unfortunately failing in 2.7 (as you can see in AppVeyor and CircleCI). Please take a look at them :-) |
OK green lights....2.7 that's still a thing :-) It was a unicode problem in the docstrings. Unfortunatly, there is still a problem in the matplotlib Axes return for python 2.7, but I'm pretty amazed it worked in 3.5 without docstrings. I guess I have to stop this PR somewhere. I'll start looking at the upstream for jedi 0.10.0 and see if I can get some of these patches in there. Now this is ready for review. |
types = docstrings.find_return_types(evaluator, self) | ||
if types: | ||
for result in types: | ||
debug.dbg('BC!!!!!!!!!!!!!! wow types?: %s in %s', result, self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you improve this debug message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe not :-) This comes from Jedi source code, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes...oops...I needed something easy to see. I'll make it better
# [2] Adding type returns for compiled objects in jedi | ||
# Patching jedi.evaluate.compiled.CompiledObject... | ||
from jedi.evaluate.compiled import \ | ||
CompiledObject, builtin, _create_from_name, debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please parenthesis here
from jedi.evaluate.compiled import (
CompiledObject, builtin, _create_from_name, debug)
# [4] Fixing introspection for matplotlib Axes objects | ||
# Patching jedi.evaluate.precedence... | ||
from jedi.evaluate.representation import \ | ||
tree, InstanceName, Instance, compiled, FunctionExecution, InstanceElement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parenthesis here too
from jedi.parser.tree import Class | ||
from jedi.common import indent_block | ||
from jedi.evaluate.iterable import Array, FakeSequence, AlreadyEvaluated | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we please arrange the imports as
# Standard library imports
from ast import literal_eval
from itertools import chain
from textwrap import dedent
import re
# Third party imports
from jedi import debug
from jedi.common import indent_block
from jedi.evaluate.cache import memoize_default
from jedi.evaluate.iterable import AlreadyEvaluated, Array, FakeSequence
from jedi.parser import Parser, load_grammar
from jedi.parser.tree import Class
I left some minor, style comments. After that, I'd say this one is ready :-) Thanks a lot for this great contribution! |
return [] | ||
|
||
def _search_return_in_numpydocstr(docstr): | ||
r""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the r here?
|
||
def _expand_typestr(p_type): | ||
""" | ||
Attempts to interpret the possible types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be
"""Attempts to interpret the possible types."""
|
||
|
||
def _evaluate_for_statement_string(evaluator, string, module): | ||
if string is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docstring?
for element in re.findall('((?:\w+\.)*\w+)\.', string): | ||
# Try to import module part in dotted name. | ||
# (e.g., 'threading' in 'threading.Thread'). | ||
string = 'import %s\n' % element + string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer the use of format and like
string = 'import {0}\n'.format(element) + string
# Take the default grammar here, if we load the Python 2.7 grammar here, it | ||
# will be impossible to use `...` (Ellipsis) as a token. Docstring types | ||
# don't need to conform with the current grammar. | ||
p = Parser(load_grammar(), code % indent_block(string)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
|
||
def _execute_array_values(evaluator, array): | ||
""" | ||
Tuples indicate that there's not just one return value, but the listed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
if isinstance(array, Array): | ||
values = [] | ||
for types in array.py__iter__(): | ||
objects = set(chain.from_iterable(_execute_array_values(evaluator, typ) for typ in types)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too long line?
@memoize_default(None, evaluator_is_first_arg=True) | ||
def follow_param(evaluator, param): | ||
""" | ||
Determines a set of potential types for `param` using docstring hints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
period at the end
return set( | ||
[p for string in _search_param_in_docstr(docstr, param_str) | ||
for p in _evaluate_for_statement_string(evaluator, string, module)] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it better to just use two normal loops?
Using a doubly nested list comp. does not make things clear.
@memoize_default(None, evaluator_is_first_arg=True) | ||
def find_return_types(evaluator, func): | ||
""" | ||
Determines a set of potential return types for `func` using docstring hints |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
period at the end
@bcolsen took |
Ok! |
I made the changes to jedi_patch.py. I really don't want to mess around with docstrings.py too much. I hope to upstream some changes to jedi after the docstrings patch gets merged there. Thanks for your encouragement on this PR and of coarse the great work you do on Spyder! |
Thanks for this great work @bcolsen :-) |
Sure it is! |
This pull request uses code from davidhalter/jedi#796 to add completions of returns that are in numpydoc doc format. This pull fixes #2162 for numpy and matplotlib.
In order for this to work you need the "numpydoc" module installed
Here is some "test" code to copy into the Spyder editor.