[GitHub] [qpid-proton] jiridanek opened a new pull request #289: PROTON-2322 Fix various flake8 warnings (manual changes)

[GitBox]

jiridanek opened a new pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

astitcher commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558361959



##########
File path: python/proton/_reactor.py
##########
@@ -60,7 +60,9 @@ def _generate_uuid():
 def _now():
     return time.time()
 
-#@total_ordering

Review comment:
       As above actually available on 2.7

##########
File path: python/proton/_reactor.py
##########
@@ -19,7 +19,7 @@
 
 from __future__ import absolute_import
 
-#from functools import total_ordering

Review comment:
       Nope total_ordering is available on 2.7 - I should have just removed the comment markers!

##########
File path: python/proton/_data.py
##########
@@ -582,31 +582,31 @@ class Data:
         * :const:`MAP`
     """
 
-    NULL = PN_NULL; "A null value."
-    BOOL = PN_BOOL; "A boolean value."
-    UBYTE = PN_UBYTE; "An unsigned byte value."
-    BYTE = PN_BYTE; "A signed byte value."
-    USHORT = PN_USHORT; "An unsigned short value."
-    SHORT = PN_SHORT; "A short value."
-    UINT = PN_UINT; "An unsigned int value."
-    INT = PN_INT; "A signed int value."
-    CHAR = PN_CHAR; "A character value."
-    ULONG = PN_ULONG; "An unsigned long value."
-    LONG = PN_LONG; "A signed long value."
-    TIMESTAMP = PN_TIMESTAMP; "A timestamp value."
-    FLOAT = PN_FLOAT; "A float value."
-    DOUBLE = PN_DOUBLE; "A double value."
-    DECIMAL32 = PN_DECIMAL32; "A DECIMAL32 value."
-    DECIMAL64 = PN_DECIMAL64; "A DECIMAL64 value."
-    DECIMAL128 = PN_DECIMAL128; "A DECIMAL128 value."
-    UUID = PN_UUID; "A UUID value."
-    BINARY = PN_BINARY; "A binary string."
-    STRING = PN_STRING; "A unicode string."
-    SYMBOL = PN_SYMBOL; "A symbolic string."
-    DESCRIBED = PN_DESCRIBED; "A described value."
-    ARRAY = PN_ARRAY; "An array value."
-    LIST = PN_LIST; "A list value."
-    MAP = PN_MAP; "A map value."

Review comment:
       Not sure about this change - for consistency maybe this should be triple quote  rather than comment? Unles the '#: ' mark is a specific thing I don't know about?

##########
File path: python/proton/_delivery.py
##########
@@ -34,7 +34,7 @@
 
 
 class NamedInt(int):
-    values = {}  # type: Dict[int, str]

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

jiridanek commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558365540



##########
File path: python/proton/_reactor.py
##########
@@ -19,7 +19,7 @@
 
 from __future__ import absolute_import
 
-#from functools import total_ordering

Review comment:
       Right. I saw "New in version 3.2." and forgot that 2 and 3 Pythons actually developed alongside...




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

jiridanek commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558366694



##########
File path: python/proton/_data.py
##########
@@ -582,31 +582,31 @@ class Data:
         * :const:`MAP`
     """
 
-    NULL = PN_NULL; "A null value."
-    BOOL = PN_BOOL; "A boolean value."
-    UBYTE = PN_UBYTE; "An unsigned byte value."
-    BYTE = PN_BYTE; "A signed byte value."
-    USHORT = PN_USHORT; "An unsigned short value."
-    SHORT = PN_SHORT; "A short value."
-    UINT = PN_UINT; "An unsigned int value."
-    INT = PN_INT; "A signed int value."
-    CHAR = PN_CHAR; "A character value."
-    ULONG = PN_ULONG; "An unsigned long value."
-    LONG = PN_LONG; "A signed long value."
-    TIMESTAMP = PN_TIMESTAMP; "A timestamp value."
-    FLOAT = PN_FLOAT; "A float value."
-    DOUBLE = PN_DOUBLE; "A double value."
-    DECIMAL32 = PN_DECIMAL32; "A DECIMAL32 value."
-    DECIMAL64 = PN_DECIMAL64; "A DECIMAL64 value."
-    DECIMAL128 = PN_DECIMAL128; "A DECIMAL128 value."
-    UUID = PN_UUID; "A UUID value."
-    BINARY = PN_BINARY; "A binary string."
-    STRING = PN_STRING; "A unicode string."
-    SYMBOL = PN_SYMBOL; "A symbolic string."
-    DESCRIBED = PN_DESCRIBED; "A described value."
-    ARRAY = PN_ARRAY; "An array value."
-    LIST = PN_LIST; "A list value."
-    MAP = PN_MAP; "A map value."

Review comment:
       It is a Sphinx syntax, https://www.sphinx-doc.org/en/1.4.8/ext/autodoc.html#directive-autoattribute
   
   The reason for the change is that multiple statements on a line separated by semicolon is against PEP 8. Flake 8 complains.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

jiridanek commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558367543



##########
File path: python/proton/_delivery.py
##########
@@ -34,7 +34,7 @@
 
 
 class NamedInt(int):
-    values = {}  # type: Dict[int, str]

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

astitcher commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558370668



##########
File path: python/CMakeLists.txt
##########
@@ -289,7 +289,7 @@ else ()
       set_tests_properties(python-tox-test
         PROPERTIES
         PASS_REGULAR_EXPRESSION "Totals: .* ignored, 0 failed"
-        FAIL_REGULAR_EXPRESSION "ERROR:[ ]+py[0-9]*: commands failed")

Review comment:
       This is probably a good change irrespective of anything else! (it's just more future proof)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

astitcher commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558371410



##########
File path: python/tox.ini
##########
@@ -25,6 +25,33 @@ deps =
 changedir = {[tox]setupdir}
 commands = flake8
 
+[flake8]
+# TODO(PROTON-2322) decrease the limit
+max-line-length = 150
+
+# TODO(PROTON-2322) re-enable all these warnings
+ignore =
+    # do not use bare 'except'
+    E722,
+    # imported but unused
+    F401,
+    # 'from proton import *' used; unable to detect undefined names
+    F403,

Review comment:
       It may not be worth adding this test until this list is much smaller!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

astitcher commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558373586



##########
File path: python/proton/_delivery.py
##########
@@ -34,7 +34,7 @@
 
 
 class NamedInt(int):
-    values = {}  # type: Dict[int, str]

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

astitcher commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558374337



##########
File path: python/proton/_data.py
##########
@@ -582,31 +582,31 @@ class Data:
         * :const:`MAP`
     """
 
-    NULL = PN_NULL; "A null value."
-    BOOL = PN_BOOL; "A boolean value."
-    UBYTE = PN_UBYTE; "An unsigned byte value."
-    BYTE = PN_BYTE; "A signed byte value."
-    USHORT = PN_USHORT; "An unsigned short value."
-    SHORT = PN_SHORT; "A short value."
-    UINT = PN_UINT; "An unsigned int value."
-    INT = PN_INT; "A signed int value."
-    CHAR = PN_CHAR; "A character value."
-    ULONG = PN_ULONG; "An unsigned long value."
-    LONG = PN_LONG; "A signed long value."
-    TIMESTAMP = PN_TIMESTAMP; "A timestamp value."
-    FLOAT = PN_FLOAT; "A float value."
-    DOUBLE = PN_DOUBLE; "A double value."
-    DECIMAL32 = PN_DECIMAL32; "A DECIMAL32 value."
-    DECIMAL64 = PN_DECIMAL64; "A DECIMAL64 value."
-    DECIMAL128 = PN_DECIMAL128; "A DECIMAL128 value."
-    UUID = PN_UUID; "A UUID value."
-    BINARY = PN_BINARY; "A binary string."
-    STRING = PN_STRING; "A unicode string."
-    SYMBOL = PN_SYMBOL; "A symbolic string."
-    DESCRIBED = PN_DESCRIBED; "A described value."
-    ARRAY = PN_ARRAY; "An array value."
-    LIST = PN_LIST; "A list value."
-    MAP = PN_MAP; "A map value."

Review comment:
       ok




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

jiridanek commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558375428



##########
File path: python/proton/_delivery.py
##########
@@ -34,7 +34,7 @@
 
 
 class NamedInt(int):
-    values = {}  # type: Dict[int, str]

Review comment:
       ```python
   >>> 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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

jiridanek commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558375913



##########
File path: python/proton/_delivery.py
##########
@@ -34,7 +34,7 @@
 
 
 class NamedInt(int):
-    values = {}  # type: Dict[int, str]

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

astitcher commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558377853



##########
File path: python/proton/_delivery.py
##########
@@ -34,7 +34,7 @@
 
 
 class NamedInt(int):
-    values = {}  # type: Dict[int, str]

Review comment:
       So make typing a dependency in the tox test so that it gets installed before running the test




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

astitcher commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558387226



##########
File path: python/proton/_delivery.py
##########
@@ -34,7 +34,7 @@
 
 
 class NamedInt(int):
-    values = {}  # type: Dict[int, str]

Review comment:
       But when you run flake8 under tox you're not specifying which version of python at all, so I don't see how it's complaining about py2




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

jiridanek commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558399078



##########
File path: python/proton/_delivery.py
##########
@@ -34,7 +34,7 @@
 
 
 class NamedInt(int):
-    values = {}  # type: Dict[int, str]

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

jiridanek commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558400559



##########
File path: python/proton/_delivery.py
##########
@@ -34,7 +34,7 @@
 
 
 class NamedInt(int):
-    values = {}  # type: Dict[int, str]

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

astitcher commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558410306



##########
File path: python/proton/_delivery.py
##########
@@ -34,7 +34,7 @@
 
 
 class NamedInt(int):
-    values = {}  # type: Dict[int, str]

Review comment:
       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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

jiridanek commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r558413304



##########
File path: python/proton/_delivery.py
##########
@@ -34,7 +34,7 @@
 
 
 class NamedInt(int):
-    values = {}  # type: Dict[int, str]

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

jiridanek commented on a change in pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289#discussion_r578524266



##########
File path: python/tox.ini
##########
@@ -25,6 +25,33 @@ deps =
 changedir = {[tox]setupdir}
 commands = flake8
 
+[flake8]
+# TODO(PROTON-2322) decrease the limit
+max-line-length = 150
+
+# TODO(PROTON-2322) re-enable all these warnings
+ignore =
+    # do not use bare 'except'
+    E722,
+    # imported but unused
+    F401,
+    # 'from proton import *' used; unable to detect undefined names
+    F403,

Review comment:
       https://issues.apache.org/jira/browse/PROTON-2336




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

jiridanek merged pull request #289:
URL: https://github.com/apache/qpid-proton/pull/289


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]