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.

Sign up for GitHub

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

Jump to bottom

Wrong processing of typing.Protocol #309

Open
akhundMurad opened this issue Apr 1, 2023 · 9 comments · May be fixed by #313
Open

Wrong processing of typing.Protocol #309

akhundMurad opened this issue Apr 1, 2023 · 9 comments · May be fixed by #313

Comments

@akhundMurad
Copy link

Suppose I have protocol class:

from typing import Protocol


class LoggerProtocol(Protocol):
    def log(
        self, level: int, msg: str, *args, extra: Mapping[str, object] | None = None
    ) -> None:
        ...

After execution of vulture I get the following output:

logging.py:24: unused variable 'args' (100% confidence)
logging.py:24: unused variable 'extra' (100% confidence)
logging.py:24: unused variable 'msg' (100% confidence)

In my opinion, vulture should ignore this type of "confidence", due to the nature of typing.Protocol
@akhundMurad akhundMurad changed the title Using with typing.Protocol Wrong processing of typing.Protocol Apr 1, 2023
@pm3512
Copy link

pm3512 commented Apr 20, 2023

Hi, can I work on this?

@jendrikseipp
Copy link
Owner

Can't you just add del args etc. in the method body @akhundMurad ?

@akhundMurad
Copy link
Author

@jendrikseipp It will make my code messy. Just imagine that each protocol method will have del statement in it...

@jendrikseipp
Copy link
Owner

Can you paste the content of the method to get a clearer picture for the use case?

Also, is it possible to change the method signature, i.e., to change msg to msg_ to signal that the argument is unused?

@akhundMurad
Copy link
Author

https://github.com/akhundMurad/diator/blob/main/src/diator/middlewares/logging.py

Here you can see the full code. I added # noqa comment as a temporary fix.

@jendrikseipp
Copy link
Owner

Thanks! And is it possible to change the method signature, i.e., to change msg to msg_ to signal that the argument is unused?

@akhundMurad
Copy link
Author

akhundMurad commented Apr 21, 2023 •
edited
Loading

The purpose of the Protocol is to define the signature of class and its methods.
Therefore, I cannot change the argument name

@jendrikseipp
Copy link
Owner

OK, thanks. Then the best solution would be to detect that this class inherits from Protocol and ignore the unused method arguments. Doing this cleanly will probably require adding scope information to Vulture, see #304. Until then, I don't think it makes sense to tackle this issue.

@akhundMurad
Copy link
Author

Got it, thank you!

@pm3512 pm3512 linked a pull request May 7, 2023 that will close this issue
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix false positives in typing.Protocol pm3512/vulture
3 participants
@jendrikseipp @pm3512 @akhundMurad