-
Notifications
You must be signed in to change notification settings - Fork 188
WIP: Use Jedi as the underlying parser #240
Conversation
…import problems) # Conflicts: # src/pydocstyle/checker.py # src/pydocstyle/parser.py # src/tests/test_definitions.py
…official API so that's my fault).
This is still in the works, but I would appreciate any thoughts or comments from anyone who has a bit of time (esp. |
I haven't looked at the code at all. My first question, however, is how does a Jedi AST differ from a Python stdlib AST? I ask specifically because Flake8 defaults to using a stdlib AST. It provides that parsed AST (doing it only once) to each plugin that traverses an AST. So if pydocstyle develops its own Flake8 plugin, it'll need to know how to take a stdlib AST and use that instead of a Jedi AST (or we'll have to work on converting Flake8 to using Jedi ASTs). |
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.
I'll probably have to do a few more passes to give you good feedback on the parser module rewrite.
setup.py
Outdated
@@ -31,6 +31,9 @@ | |||
keywords='pydocstyle, PEP 257, pep257, PEP 8, pep8, docstrings', | |||
packages=('pydocstyle',), | |||
package_dir={'': 'src'}, | |||
install_requires=[ | |||
'jedi==0.9.0', |
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, let's not pin this. This causes unending headaches for users and downstream redistributors.
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.
Eventually this will not be pinned. I temporarily pinned this because a new jedi
was released while I was working on this, which broke some of my changes.
src/__init__.py
Outdated
@@ -0,0 +1 @@ | |||
"""Pydocstyle - a docstring style checker.""" |
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.
Is src/
meant to be a package? If yes, why? To be clear, I don't think src/
should be a package.
@@ -680,6 +681,10 @@ def check(filenames, select=None, ignore=None, ignore_decorators=None): | |||
try: | |||
with tokenize_open(filename) as file: | |||
source = file.read() | |||
try: | |||
source = source.decode('utf-8') | |||
except: |
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.
I think this except should be more narrowly scoped. There are a surprising number of exceptions that can happen here and they depend on platform, how the python interpreter is compiled, etc.
src/tests/__init__.py
Outdated
@@ -0,0 +1 @@ | |||
"""Test for pydocstyle.""" |
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.
py.test does not require an __init__.py
and often recommends you don't make your tests module importable.
@@ -80,9 +94,16 @@ def _publicity(self): | |||
return {True: 'public', False: 'private'}[self.is_public] | |||
|
|||
@property | |||
def source_lines(self): | |||
if '_source' in self._fields: |
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.
Will _source
ever be in _fields
now? Can't you just make this property a proxy for self.parent.source_lines
now?
|
||
class DunderAll(Value): | ||
"""A list of exported names in a module.""" | ||
_fields = 'names'.split() |
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.
>>> 'names'.split()
['names']
Why not simply do?
_fields = ['names']
|
||
class FutureImport(Value): | ||
"""A list of __future__ imports in a module.""" | ||
_fields = 'name'.split() |
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 question.
self.current.value.split('noqa: ')[1:]) | ||
elif self.current.value.startswith('# noqa'): | ||
skipped_error_codes = 'all' | ||
if 'noqa: ' in comment: |
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.
If I stripped the noqa
logic into a separate library, would you use that instead?
@@ -680,6 +681,10 @@ def check(filenames, select=None, ignore=None, ignore_decorators=None): | |||
try: | |||
with tokenize_open(filename) as file: | |||
source = file.read() | |||
try: | |||
source = source.decode('utf-8') | |||
except: |
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.
To catch any exception use the except Exception
, instead of except BaseException
or just except
.
Indicate the most specific exception type in except
block.
For example:
# Bad
try:
mapping[key]
except Exception:
...
# Better
try:
mapping[key]
except KeyError:
...
…e pinning before merge).
Closing this as it has been inactive for a while. Please feel free to re-open if this is still relevant. |
WIP, description to come.