Add isort (for sorting imports) #227

mdesmet · 2022-08-19T12:42:35Z

Introduces isort (sorting imports)

Added isort as "test" dependencies
Run pre-commit run --all-files to standardize formatting and import sorting.

nineinchnick · 2022-08-19T12:44:40Z

setup.py

@@ -16,7 +16,7 @@
 import re
 import textwrap

-from setuptools import setup, find_packages
+from setuptools import find_packages, setup


Formatting changes should be in a separate commit.

A separate commit would fail the build if ran by itself.

Why? Make it the first commit.

hashhar

Reformatting all in one go loses quite a lot of history.

+1 to adding black and isort though.

I'll defer to others on suggestions.

hashhar · 2022-08-19T12:55:55Z

.flake8

+select =
+    E
+    W
+    F
+ignore =
+    W503 # makes Flake8 work like black
+    W504
+    E203 # makes Flake8 work like black
+    E741
+    E501


ideally each line should have comments to explain what these do

README.md

hashhar · 2022-08-19T13:03:20Z

README.md

+
+Following checks are performed:
+
+- [`flake8`](https://flake8.pycqa.org/en/latest/) for code linting


linting should run on CI for now. We don't want to prevent from submitting PRs or pushing changes just when experimenting or when one needs help to fix whatever is being complained about.

That can be disabled by git commit . -m 'quick fix' --no-verify. We can add this to the docs as well.

Note that in general only mypy might stop you. black and isort just autoformat your files.

IMHO in general it will be better to always run it on commit to prevent contributors from the experience of failing the build for easily fixable problems.

WDYT?

I agreed with black and isort. I just meant to keep flake8 and mypy outside of pre-commit for now. We can iterate.

hashhar · 2022-08-19T13:03:40Z

README.md

+- [`flake8`](https://flake8.pycqa.org/en/latest/) for code linting
+- [`black`](https://github.com/psf/black) for code formatting
+- [`isort`](https://pycqa.github.io/isort/) for sorting imports
+- [`mypy`](https://mypy.readthedocs.io/en/stable/) for static type checking


mypy on CI only for now as well.

hashhar · 2022-08-19T13:04:16Z

tests/integration/conftest.py

-            user="test_fixture"
-        )
-    )
+    request = TrinoRequest(host=host, port=port, client_session=ClientSession(user="test_fixture"))


arguably less readable than before.

This seems linked to the setting of the line length. It seems black wants to shove as much on the same line if permitted by the line length. No other knobs available. black is fairly opinionated.

@mdesmet if you add a trailing comma after client_session=ClientSession(user="test_fixture"), Black will format the code is a similar way to how it was formatted previously.

hashhar · 2022-08-19T13:07:38Z

tests/integration/test_dbapi_integration.py

+            """
+        )


Changed to one liner with single quotes

hashhar · 2022-08-19T13:09:14Z

tests/integration/test_sqlalchemy_integration.py

+        and_(
+            customers.c.name.like("%12%"),
+            customers.c.nationkey == 15,
+            or_(customers.c.mktsegment == "AUTOMOBILE", customers.c.mktsegment == "HOUSEHOLD"),
+            not_(customers.c.acctbal < 0),


Does black reformat this away?

Suggested change

and_(

customers.c.name.like("%12%"),

customers.c.nationkey == 15,

or_(customers.c.mktsegment == "AUTOMOBILE", customers.c.mktsegment == "HOUSEHOLD"),

not_(customers.c.acctbal < 0),

and_(

customers.c.name.like("%12%"),

customers.c.nationkey == 15,

or_(customers.c.mktsegment == "AUTOMOBILE", customers.c.mktsegment == "HOUSEHOLD"),

not_(customers.c.acctbal < 0),

Actually the _or(...) is an argument to the _and function, so this seems correct.

hashhar · 2022-08-19T13:10:00Z

tests/unit/conftest.py

@@ -194,8 +195,7 @@ def sample_get_error_response_data():
            "errorType": "USER_ERROR",
            "failureInfo": {
                "errorLocation": {"columnNumber": 15, "lineNumber": 1},
-                "message": "line 1:15: Schema must be specified "
-                "when session schema is not set",
+                "message": "line 1:15: Schema must be specified " "when session schema is not set",


Suggested change

"message": "line 1:15: Schema must be specified " "when session schema is not set",

"message": "line 1:15: Schema must be specified when session schema is not set",

hashhar · 2022-08-19T13:10:32Z

tests/unit/oauth_test_utils.py

+                "Www-Authenticate": f'Bearer x_redirect_server="{self.redirect_server}", '
+                f'x_token_server="{self.token_server}"',
+                "Basic realm": '"Trino"',


More readable and "safer" to have entire token on one line

Fixed by increasing line length.

hashhar · 2022-08-19T13:10:57Z

tests/unit/sqlalchemy/test_compiler.py

+    assert (
+        str(query)
+        == 'SELECT "table".id, "table".name \nFROM "table"\nOFFSET :param_1\nLIMIT :param_2'
+    )


questionable redability

A multi line string would be more fitting here. However that triggers flake8 as it seems flake8 doesn't allow spaces at EOL, even within multi line strings.

hashhar · 2022-08-19T13:30:51Z

.pre-commit-config.yaml

+    hooks:
+      - id: black
+        args:
+          - "--line-length=99"


enforcing a specific line length can hinder readability - line length limits don't provide any benefits IMO and sometimes discourage from good naming or formatting SQL queries in an idiomatic way.

Can we make this length infinite?

hovaesco

LGTM % black complains in CI

hovaesco · 2022-08-22T11:26:05Z

.pre-commit-config.yaml

@@ -4,6 +4,7 @@ repos:
    hooks:
      - id: "flake8"
        name: "Python: analysis"
+        stages: [manual]


all might be more descriptive, since it runs all checks?

Actually we want to restrict flake8 and mypy to not be run on any stage except in CI.

See https://pre-commit.com/#confining-hooks-to-run-at-certain-stages

john-bodley · 2022-08-31T05:43:11Z

tests/integration/conftest.py

-            user="test_fixture"
-        )
-    )
+    request = TrinoRequest(host=host, port=port, client_session=ClientSession(user="test_fixture"))


@mdesmet if you add a trailing comma after client_session=ClientSession(user="test_fixture"), Black will format the code is a similar way to how it was formatted previously.

john-bodley · 2022-08-31T05:45:45Z

.pre-commit-config.yaml

+    rev: 5.6.4
+    hooks:
+      - id: isort
+        args: [ "--profile", "black", "--filter-files" ]


Why are these args required? I've used the isort pre-commit successfully with a number of projects which haven't required any additional arguments.

The filter-files can be omitted. I think it's just there to not change any unmodified files, even if explicitly asked.

The --profile "black" actually formats the imports differently

with the black profile

from trino.exceptions import ( DatabaseError, DataError, Error, IntegrityError, InterfaceError, InternalError, NotSupportedError, OperationalError, ProgrammingError, Warning, )

without the profile

from trino.exceptions import (DatabaseError, DataError, Error, IntegrityError, InterfaceError, InternalError, NotSupportedError, OperationalError, ProgrammingError, Warning)

I personally like the black profile more as it is faster to scan.

john-bodley · 2022-08-31T05:50:29Z

.pre-commit-config.yaml

+    hooks:
+      - id: black
+        args:
+          - "--line-length=120"


Any reason to override the default 88 character line length? I realize it's somewhat arbitrary what the line length is if you have an auto-formatter, but as far as I can tell most packages adhere to the default.

Because line length doesn't correlate to any reasonable thing. It's an arbitrary rule to be followed and if you'd take a look at the reformats black did with shorter line lengths have questionable readability.

mdesmet · 2022-09-01T11:50:01Z

Some flakyness in the tests, might be related to not having merged #220 but not totally sure.

hashhar · 2022-09-12T09:00:00Z

@mdesmet See also cffd2b2#r83714277 - not sure if that's the cause.

mdesmet · 2022-10-27T12:41:11Z

@hashhar: As discussed I have removed black from this PR and only kept isort. PTAL

hashhar · 2022-10-31T07:21:02Z

.flake8

@@ -0,0 +1,12 @@
+[flake8]


intended to be part of this PR or leftover? I think leftover?

hashhar · 2022-10-31T07:22:47Z

.github/DEVELOPMENT.md

@@ -25,6 +25,18 @@ With `-e` passed to `pip install` above pip can reference the code you are
 modifying in the *virtual env*. That way, you do not need to run `pip install`
 again to make your changes applied to the *virtual env*.

+### `pre-commit` checks


What exact checks are run will change over time - doesn't seem as important.

Maybe we'd want to instead mention about pre-commit install at Code style header which already tells how to run these checks manually.

mdesmet · 2022-10-31T08:32:28Z

@hashhar: PTAL

cla-bot bot added the cla-signed label Aug 19, 2022

nineinchnick reviewed Aug 19, 2022

View reviewed changes

nineinchnick approved these changes Aug 19, 2022

View reviewed changes

mdesmet requested review from hovaesco, hashhar and ebyhr August 19, 2022 12:47

mdesmet force-pushed the feature/auto-formatting branch from 073acb7 to 18757d9 Compare August 19, 2022 12:55

hashhar reviewed Aug 19, 2022

View reviewed changes

mdesmet force-pushed the feature/auto-formatting branch 3 times, most recently from 5ca3524 to d681e77 Compare August 22, 2022 09:55

hovaesco approved these changes Aug 22, 2022

View reviewed changes

mdesmet force-pushed the feature/auto-formatting branch from d681e77 to 3ee643e Compare August 22, 2022 11:35

john-bodley reviewed Aug 31, 2022

View reviewed changes

mdesmet force-pushed the feature/auto-formatting branch from 3ee643e to e3e83b5 Compare September 1, 2022 11:34

hovaesco mentioned this pull request Sep 23, 2022

Improve contribution process #239

Merged

mdesmet force-pushed the feature/auto-formatting branch from e3e83b5 to 4aec2b4 Compare October 27, 2022 12:13

mdesmet changed the title ~~Added black (code formatting) and isort (sorting imports)~~ Add isort (for sorting imports) Oct 27, 2022

hashhar reviewed Oct 31, 2022

View reviewed changes

mdesmet added 3 commits October 31, 2022 09:05

Sort imports using isort

90f0358

Add isort autoformatting using pre-commit

0b742ee

Add pre-commit instructions in DEVELOPMENT guide

bcadd77

mdesmet force-pushed the feature/auto-formatting branch from 4aec2b4 to bcadd77 Compare October 31, 2022 08:06

hashhar approved these changes Oct 31, 2022

View reviewed changes

hashhar added the no-release-notes This pull request does not require release notes entry label Oct 31, 2022

hashhar merged commit 0399b86 into trinodb:master Oct 31, 2022

mdesmet deleted the feature/auto-formatting branch October 31, 2022 09:43


		Following checks are performed:

		- [`flake8`](https://flake8.pycqa.org/en/latest/) for code linting

	"message": "line 1:15: Schema must be specified " "when session schema is not set",
	"message": "line 1:15: Schema must be specified when session schema is not set",

Add isort (for sorting imports) #227

Add isort (for sorting imports) #227

Conversation

mdesmet commented Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

hovaesco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdesmet commented Sep 1, 2022

hashhar commented Sep 12, 2022

mdesmet commented Oct 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdesmet commented Oct 31, 2022

mdesmet commented Aug 19, 2022 •

edited

Loading

john-bodley Aug 31, 2022 •

edited

Loading

hashhar Aug 19, 2022 •

edited

Loading

john-bodley Aug 31, 2022 •

edited

Loading