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

initial poc pep-0484 type hints #659

Closed
wants to merge 1 commit into from
Closed

initial poc pep-0484 type hints #659

wants to merge 1 commit into from

Conversation

reinhrst
Copy link
Contributor

I'm taking a swipe at #170 , and this is just to see if I'm on the right track. jedi/evaluate/pep0484.py contains a checklist with functionality that needs to be implemented on this end (I believe that the first 4 or 5 should be relatively straight-forward to implement, maybe even all with exception of the stub files, especially if they live in a different directory). Then in addition, jedi needs to understand all the subtleties of the typing module, but that's another chapter. Even the current very small change would already fix 50% of the times that I don't get completions in my current project.

Things that I'd like some feedback on (although feel free to shoot at other things as well):

  • I'm currently getting the annotations from the parsed code. Alternatively one could ask the python interpreter and use the .__annotations__. Which is preferred?
  • My if and assert statements to look for annotations seems to hold in all the tests (in python 3.4). Are the assumptions there correct? Should I perhaps use some less-defensive programming style, to allow for future python-syntax additions?
  • I can't get tox to run, so only ran the tests in python 3.4, but I'm pretty sure the test will fail in 2.x because of the syntax. How do I guard the tests such that they only run in >=3.2 ?
  • Right now I have annotations take precedence over docstring in case they are present. Is this the way to go, or should we perhaps merge them?
  • PEP 0484 supports decorators for switching off type hinting in some modules/functions. For now I didn't include them (even on the todo list) since worst case one will just get wrong completions, right? Or should we allow for them?

""" Pep-0484 type hinting """

# -----------------
# sphinx style
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that's what you get for copy-pasting other files :(

@davidhalter
Copy link
Owner

If it's not a big hassle for you, I would be very grateful if you start working
on this on the linter branch. I'm going to merge it upstream very soon. This
would be easier for you (easier to understand) as well.

I'm currently getting the annotations from the parsed code. Alternatively one could ask the python interpreter and use the .annotations. Which is preferred?

Jedi only supports the parser, you cannot just magically use __annotations__.
However we still want type inference there. What I would suggest is something
that already exists within Jedi in some other forms (e.g. py__mro__,
py__bases, etc) - we recreate __annotations__ in a static analysis kind of
way (https://github.com/davidhalter/jedi/blob/linter/jedi/evaluate/representation.py#L17):

'


class Function():  # In evaluate.representation
    @memoize_default()
    def py__annotations__(self):
        parser_func = self.base
        dct = {'return': self._evaluator.eval_element(parser_func.annotation)}
        for param in parser_func.params:
            dct[param.name.value] = self._evaluator.eval_element(param.annotation)
        return dct

With this function you basically have everything you need.

My if and assert statements to look for annotations seems to hold in all the tests (in python 3.4). Are the assumptions there correct? Should I perhaps use some less-defensive programming style, to allow for future python-syntax additions?

Don't care for future additions. That's currently fine. Any future
modifications should be tested separately. But as hinted above, you shouldn't
be using the parser tree that much in the first place. Just use the
annotation properties (they are already defined in parser.tree). I think you stil have to implementParam.annotation`, though.

I can't get tox to run, so only ran the tests in python 3.4, but I'm pretty sure the test will fail in 2.x because of the syntax. How do I guard the tests such that they only run in >=3.2 ?

Very good question again. Yes we need to limit it somehow. I don't know yet how
we would do that best. If you have a clever idea, let me know that. (Maybe
something like py > 2 in the first line of the file? And then parser for it?)
But if you're not checking for that, I will still merge. It's good enough if
the tests for any Python version passes. I can then do the rest from there.

Right now I have annotations take precedence over docstring in case they are present. Is this the way to go, or should we perhaps merge them?

Wow you're on the right track! :) I'm not sure if I can really answer that.
What do you think? I would tend to say they hold about the same level of
priority.

PEP 0484 supports decorators for switching off type hinting in some modules/functions. For now I didn't include them (even on the todo list) since worst case one will just get wrong completions, right? Or should we allow for them?

I agree that ignoring them for now is probably best. This would be a "goodie"
in the end, but we really don't need to do that now. I think they are very
rarely used, anyways.

@reinhrst
Copy link
Contributor Author

Will open new PR on that branch.

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.

None yet

2 participants