Added flake8 test and updated affected code #68
Conversation
| Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
1 similar comment
| Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
joshthoward force-pushed the flake8-test branch from 1c9608f to c0c0d93 Compare | Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
| CLA has been sent via email. |
hashhar left a comment
hashhar left a comment There was a problem hiding this comment.
A few comments.
Thanks for your contribution. ❤️
| | ||
| @pytest.mark.skip(reason="Nan currently not returning the correct python type fon inf") | ||
| def test_float_inf_query_param(trino_connection): | ||
| cur = trino_connection.cursor() |
integration_tests/test_dbapi.py Outdated
| cur.execute( | ||
| """ | ||
| select * FRMO foo WHERE x = ?; | ||
| select * FRMO foo WHERE x = ?; |
There was a problem hiding this comment.
While we are at it, can you make SQL queries use uppercase keywords (or at-least be consistent within a single query) if it's not too much of an effort?
It should be a separate commit though.
| from . import exceptions | ||
| from . import logging | ||
| | ||
| __all__ = ['auth', 'dbapi', 'client', 'constants', 'exceptions', 'logging'] |
There was a problem hiding this comment.
I'm not sure if we consider all of these to be public API.
Most notably client isn't supposed to be public since it's an implementation detail of the driver. People can use it but it's not expected to be stable (which is what __all__ indicates by convention.
auth, dbapi, constants and exceptions might be considered public.
There was a problem hiding this comment.
__all__ is updated here to indicate to flake8 that the unused imports are part of a public API. Otherwise, we might want to remove client and logging from __init__.py. What do you think?
There was a problem hiding this comment.
Good point. Let's leave it out of this PR - I'll follow up on this in #43
| @@ -34,14 +34,6 @@ | |||
| TRINO_PORT = 8080 | |||
| | |||
| | |||
| def is_process_alive(pid): | |||
There was a problem hiding this comment.
It's surprising that this is unused. I'll check what changed to make this unused.
There was a problem hiding this comment.
It's interesting code. ProcessLookupError is not imported and cannot be caught as an exception.
There was a problem hiding this comment.
This function was never called but has been here since the beginning. 🤷♂️
Let's remove it then. 👍
joshthoward force-pushed the flake8-test branch from c0c0d93 to c822af6 Compare | Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
1 similar comment
| Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
| def test_select_query(trino_connection): | ||
| cur = trino_connection.cursor() | ||
| cur.execute("select * from system.runtime.nodes") | ||
| cur.execute("SELECT * FROM system.runtime.nodes") |
There was a problem hiding this comment.
It is not since it isn't "Python" code.
setup.cfg Outdated
| # https:#www.flake8rules.com/rules/W503.html | ||
| # https:#www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator |
joshthoward force-pushed the flake8-test branch from 2a533a0 to 1f95d39 Compare 
joshthoward force-pushed the flake8-test branch from daf8585 to fc25743 Compare 
findepi left a comment
findepi left a comment There was a problem hiding this comment.
LGTM!
One minor comment about commit boundaries.
joshthoward force-pushed the flake8-test branch from fc25743 to f342927 Compare 
| Merged, thanks! |
3 participants
Added flake8 to the GitHub CI tests and then updated the code to pass flake8 test. Some unused imports were removed.
The max line length is set to 120 and the W503 rule is ignored which is PEP 8 compliant.