Skip to content

Conversation

@jiridanek
Copy link
Contributor

No description provided.

@jiridanek jiridanek self-assigned this Jan 15, 2021
@jiridanek jiridanek requested a review from astitcher January 15, 2021 13:56


class NamedInt(int):
values = {} # type: Dict[int, str]
Copy link
Member

Choose a reason for hiding this comment

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

Remove the extra comment:
Type annotations are comments deliberately so they don't need changing fixing etc.
But my read of the doc is that this is available on 2.7 anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flake 8 looks inside # type: comments and complains if it is using something that wasn't declared; I'd leave the comment, type annotations are awesome and when Python 2.7 is dropped, they can be used more fully.

Copy link
Member

Choose a reason for hiding this comment

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

The doc for mypy says that Dict is available in typing for 2.7 - not sure what's going on here, but I strongly think we should ignore or silence flake8 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

>>> import typing
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named typing

Has to be installed with pip; it is part of stdlib only since 3.5 or maybe 3.6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and the comment I put there is silencing flake8, thats the # noqa part of it

Copy link
Member

Choose a reason for hiding this comment

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

Also re type comments - I'm much more interested in typeshed (as in separate) function level type annotations - I think I put that comment there as a helpful piece of user level info to help me understand the code.
I don't think having flake8 check this is very useful tbh, so just stop it checking type annotation for the moment.
If I want type checking I'm going to use mypy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whether flake8 runs under pyhotn2 or python3 probably plays a role here. In Python 3 code, you are supposed to have that from typing import Dict; if you forget, it's an error that flake8 can help you with.

The flake8 error this produced is

30: ./proton/_delivery.py:37:18: F821 undefined name 'Dict'

so exactly same result as if you used that name in your code. I don't see a way to tell flake8 not to behave like this, except with the # noqa comment. There is only a single type annotation like this. I strongly believe that what's in the PR is adequate until Proton moves to 3.6. Then we can simply do from typing import and we'll do type annotations the 3.x way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One reason we may want this flake8 behavior is that it quickly finds typos in type annotations. Mypy runs significantly slower.

Copy link
Member

Choose a reason for hiding this comment

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

Just add the # noqa without the extra comment then - that maintains the informal comment nature. I care much less about this sort of annotation than function level which will be useful to people using the library in an IDE

Copy link
Contributor Author

@jiridanek jiridanek Jan 15, 2021

Choose a reason for hiding this comment

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

I wanted to mark the line for revisiting when we drop Python 2.7. Because then I can delete the # noqa and add from typing import Dict at the top of the file, and maybe rewrite the type annotation to PEP 526 syntax.

@jiridanek jiridanek merged commit 6897e8e into apache:master Feb 20, 2021
@jiridanek jiridanek deleted the jd_2021_01_15_autopep_enable_flake8_tox branch February 20, 2021 13:58
jiridanek added a commit to jiridanek/qpid-proton that referenced this pull request Mar 3, 2023
* PROTON-2320 Configure and enable flake8 in tox
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants