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

Fix false positives in abstract methods #35

Merged
merged 7 commits into from
Jun 27, 2023

Conversation

jsh9
Copy link
Owner

@jsh9 jsh9 commented Jun 27, 2023

Partially tackles #31

@jsh9 jsh9 merged commit 1a3596f into main Jun 27, 2023
13 checks passed

for decorator in node.decorator_list:
if isinstance(decorator, ast.Name):
if decorator.id == 'abstractmethod':
Copy link

Choose a reason for hiding this comment

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

You could also use __isabstractmethod__ here (https://docs.python.org/3/library/abc.html#abc.abstractmethod), which might be slightly more robust in case another decorator has been used (e..g @abstractproperty, even though it's now deprecated)

Copy link
Owner Author

@jsh9 jsh9 Jun 28, 2023

Choose a reason for hiding this comment

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

You actually touched on an interesting implementation detail of pydoclint.

Pydoclint performs static syntax analysis using Python's ast module. "Static" means that the Python code is not actually run when ast parses it.

For example, when ast parses this:

@abstractmethod
def something(self, arg):
    pass

It does not actually know that it is an abstract method. It only knows that the method something() has a decorator called "abstractmethod".

If you attempt to do this:

staticmethod = classmethod

@staticmethod
def something(self, arg):
    pass

ast will only know that the method something() has a decorator called "staticmethod".

By now you must have realized that there is a type of potential edge cases: if people do hello = staticmethod, world = abstractmethod, etc., pydoclint will get confused and throw false positive violations.

But in reality, very few people do that. (Would you?)

To be able to understand that "hello" means "staticmethod" in the code, we'd need to use Python's official inspect module. I chose not to, because using inspect to parse source code is much slower than using ast. (This is not difficult to understand: with inspect, we need to first get the source code running.)

Darglint uses inspect (see code examples here and here). I suspect this is why darglint is very slow.

What I plan to do is to summarize what I said above into a disclaimer section in pydoclint's documentation, so that users know what pydoclint is not designed to handle.

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