Add linters configuration, reformat whole code #503

antoni-szych-rtbhouse · 2023-12-14T13:04:43Z

Hi,

As black and isort became the standard tools to enforce uniform style within the whole codebase, I've came to a conclusion that these may be helpful also here, as the style currently varies in different parts of the codebase.

I've added:

black
isort
moved the pytest config to pyproject.toml

I've reformated the code according to black and isort configuration.

Topics that are very open for discussion:

line lengths. Black gives default of 88 characters per line. ~~Given that nearly all displays currentlyt have 16:9 / 16:10 aspect ratio instead of 4:3, 120 characters per line also seems reasonable (which I've set in the configuration in pyproject.toml)~~ Default of 88 chars is used. However this setting is entirely dependent on the personal preference and can be easily changed to any other value. more reading
string normalization. Black by default converts all strings that do not contain double quotes " to double-quoted ones instead of singe-quoted '. However, some people have strong opinion regarding quoting style and allow double-quotes only in docstrings. I've left the default normalization on, but performed the normalization in separate commit. If needed, that commit can be easily skipped and the configuration can be changed to include skip-string-normalization. more reading
String normalization was dropped from this PR.

ds-cbo · 2023-12-14T13:57:47Z

"Given that nearly all displays currentlyt have 16:9 / 16:10 aspect ratio instead of 4:3, 120 characters per line also seems reasonable"

Counterpoint: I often have 2 terminals side-to-side on my 16:9 screen, and then it only fits up to 100 characters comfortably. It also depends on font-size, which is still very personal nowadays. I would keep the default value of black (80 + 10% wrap margin)

ds-cbo · 2023-12-14T14:01:12Z

"I've left the default normalization on"

I don't have strong opinions on quotes, but I do have a strong opinion on git history: Please try to keep the changes minimal when introducing a new tool. Other developers will git annotate a file and 50% of the lines will get "black 2023/12/14" as annotation, which is completely useless.

ds-cbo · 2023-12-14T14:02:49Z

voluptuous/__init__.py

 from voluptuous.util import *
-from voluptuous.error import *
+from voluptuous.validators import *


This might actually be a breaking change if anything in .error depends on anything in .validators (which is likely), I would disable isort for __init__.py files

from voluptuous.error import * # isort: skip

made error go to the bottom of imports

ds-cbo · 2023-12-14T14:04:03Z

voluptuous/schema_builder.py

+    object,
+    dict,
+    None,
+    typing.Callable,


this had intentional compact formatting (similar types on the same line), please wrap it in # fmt: off and # fmt: on

ds-cbo · 2023-12-14T14:04:59Z

voluptuous/schema_builder.py

+        (4, is_type),  # types/classes lowest before Extra
+        (3, is_callable),  # callables after markers
+        (5, is_extra),
+    ]  # Extra lowest priority


move this comment back to the respective line

sure thing.

ds-cbo · 2023-12-14T14:05:51Z

voluptuous/schema_builder.py

-    arguments = dict((arg_name, arg_value_list[i])
-                     for i, arg_name in enumerate(arg_names)
-                     if i < len(arg_value_list))
+    arguments = dict((arg_name, arg_value_list[i]) for i, arg_name in enumerate(arg_names) if i < len(arg_value_list))


I wouldn't say that this is more readable now, please wrap it back to several lines (decreasing line length as per my other suggestion would probably help already)

Yes, if there's a way to default to having comprehensions wrap on the keywords, that would be great.

limiting the line length to default 88 did the trick

ds-cbo · 2023-12-14T14:06:35Z

voluptuous/tests/tests.py

+    UrlInvalid,
+    raises,
+    validate,
+)


please compact this down as well, with # fmt: off and # fmt: on to keep it in place
no one likes a 50-line import statement

ds-cbo · 2023-12-14T14:08:04Z

voluptuous/tests/tests.py

-                                          extra=ALLOW_EXTRA))},
-                    extra=ALLOW_EXTRA)
+    schema = Schema(
+        {"number": int, "follow": All(Self, Schema({"extra_number": int}, extra=ALLOW_EXTRA))}, extra=ALLOW_EXTRA


this also didn't get any clearer, please expand it again (might be able to get black to do that with a trailing comma)

limiting the line length to default 88 did the trick

ds-cbo · 2023-12-14T14:09:02Z

voluptuous/validators.py

+    TrueInvalid,
+    TypeInvalid,
+    UrlInvalid,
+)


please compact this one down as well, 20-line imports are still no fun

Is there a way to change this as a default configuration option, to not have one import per line?

yep, there is (in isort), multi_line_output = 5 - however it's incompatible with black if it gets longer than line_length. # fmt: off and # fmt: on to exclude it from black formatting gets the job done.

alecthomas · 2023-12-15T00:50:53Z

voluptuous/schema_builder.py

+            key
+            for key in schema
+            if key is not Extra
+            and ((self.required and not isinstance(key, (Optional, Remove))) or isinstance(key, Required))


I don't love having the and at the same level as the if. Is there a way to configure that?

I'm afraid it's not configurable. Black has (by design and purpose) very limited configuration options, boiling down to:

line length

string quotes normalization on or off

"magic trailing comma" on or off

The upside is that you don't have to choose and maintain your own formatting style in a project.

alecthomas · 2023-12-15T00:52:20Z

voluptuous/schema_builder.py

@@ -937,8 +954,7 @@ class Msg(object):

    def __init__(self, schema: Schemable, msg: str, cls: typing.Optional[typing.Type[Error]] = None) -> None:
        if cls and not issubclass(cls, er.Invalid):
-            raise er.SchemaError("Msg can only use subclases of"
-                                 " Invalid as custom class")
+            raise er.SchemaError("Msg can only use subclases of" " Invalid as custom class")


Weird that it didn't automatically concatenate these two strings.

I've had this before, the thing with black is that it 100% guarantees that the AST stays the exact same between transformations, and merging two strings would break that promise on python 3.7- (see psf/black#26 )

But I think it should detect that we're not running on python 3.7- and merge them, so I'm not sure what's going on there

You're right, it does not out of the box (yet, but it should starting with black 24). I will run it with the mentioned experimental flag and manually check output before commiting.

flake8-implicit-str-concat would be useful for such cases, I can add it to the flake8 tox env if you agree.
I'm using pylint on a daily basis, it also has this rule

alecthomas · 2023-12-15T00:53:37Z

voluptuous/schema_builder.py

-    arguments = dict((arg_name, arg_value_list[i])
-                     for i, arg_name in enumerate(arg_names)
-                     if i < len(arg_value_list))
+    arguments = dict((arg_name, arg_value_list[i]) for i, arg_name in enumerate(arg_names) if i < len(arg_value_list))


Yes, if there's a way to default to having comprehensions wrap on the keywords, that would be great.

alecthomas · 2023-12-15T00:54:18Z

voluptuous/validators.py

+    TrueInvalid,
+    TypeInvalid,
+    UrlInvalid,
+)


Is there a way to change this as a default configuration option, to not have one import per line?

alecthomas · 2023-12-15T00:55:04Z

voluptuous/validators.py

 )
 DOMAIN_REGEX = re.compile(
    # start anchor, because fullmatch is not available in python 2.7
    "(?:"
    # domain
-    r'(?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\.)+'
-    r'(?:[A-Z]{2,6}\.?|[A-Z0-9-]{2,}\.?$)'
+    r"(?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\.)+" r"(?:[A-Z]{2,6}\.?|[A-Z0-9-]{2,}\.?$)"


Please put these back on multiple lines for readability.

sure. Note that this regex is long obsolete due to tlds like .basketball, .google or .youtube:

https://en.wikipedia.org/wiki/List_of_Internet_top-level_domains#ICANN-era_generic_top-level_domains

https://en.wikipedia.org/wiki/List_of_Internet_top-level_domains#Brand_top-level_domains

alecthomas · 2023-12-15T01:52:09Z

Counterpoint: I often have 2 terminals side-to-side on my 16:9 screen, and then it only fits up to 100 characters comfortably. It also depends on font-size, which is still very personal nowadays. I would keep the default value of black (80 + 10% wrap margin)

Counter-counterpoint: I suspect the majority of people, myself included, prefer 100 or 120 columns, and for the minority who don't, having to scroll horizontally every now and then is not the end of the world. I have to do it myself sometimes on smaller screens, and I'm fine with it.

Edit: additionally, 100+ columns is what Voluptuous is currently using.

antoni-szych-rtbhouse · 2023-12-15T12:22:50Z

"I've left the default normalization on"

I don't have strong opinions on quotes, but I do have a strong opinion on git history: Please try to keep the changes minimal when introducing a new tool. Other developers will git annotate a file and 50% of the lines will get "black 2023/12/14" as annotation, which is completely useless.

This can be actually easily fixed both locally and for github blame: we just need to put the commit sha into the .git-blame-ignore-revs file. Note that it will have to be added after merging this PR, as we don't know the merge commit sha in advance :)

Note that the same logic applies to any refactoring effort like using f-strings or getting rid of object in class Foo(object) (redundant in python3).
Even if .git-blame-ignore-revs wouldn't exist (which it does since git 2.23 from 2019), I don't think that more complicated browsing file history should really prevent anyone from code refactoring. Especially if the "complicated way" involves clicking 1 button more:

github:

pycharm:

vscode + git lens (the most popular git extension for vscode):

antoni-szych-rtbhouse · 2024-01-15T10:04:57Z

Hi @ds-cbo @alecthomas can you please take a look at the updated version? :)

ds-cbo · 2024-01-15T10:52:46Z

I have no new complaints, but I'm also not responsible for this project

antoni-szych-rtbhouse · 2024-01-30T13:57:39Z

@alecthomas could you please take a look? :) I think all the major obstacles were resolved with the change in line length and few fmt:off/ons and ignores.

alecthomas · 2024-01-30T19:16:42Z

Mmm yeah, one last change please: can you make the default string quoting ' instead of ". That is the default in this project, and leaving it as-is will reduce the diff size considerably.

alecthomas · 2024-01-31T11:04:29Z

voluptuous/validators.py

@@ -33,42 +40,42 @@
    # start anchor, because fullmatch is not available in python 2.7
    "(?:"
    # dot-atom
-    r"(^[-!#$%&'*+/=?^_`{}|~0-9A-Z]+"
-    r"(\.[-!#$%&'*+/=?^_`{}|~0-9A-Z]+)*$"
+    r"(^[-!#$%&'*+/=?^_`{}|~0-9A-Z]+(\.[-!#$%&'*+/=?^_`{}|~0-9A-Z]+)*$"


Why did this get joined? Can we split this again please?

because these 2 lines can fit into one line, black merged them into:

r"(^[-!#$%&'*+/=?^_`{}|~0-9A-Z]+" r"(\.[-!#$%&'*+/=?^_`{}|~0-9A-Z]+)*$"

which raises obvious issue "why the strings were not concatenated?", which results in:

r"(^[-!#$%&'*+/=?^_`{}|~0-9A-Z]+(\.[-!#$%&'*+/=?^_`{}|~0-9A-Z]+)*$"

The only way to keep the original version is # fmt: on and # fmt: off. I've added it, so these lines are back to being separate.

alecthomas · 2024-01-31T11:04:40Z

voluptuous/validators.py

    r'(?:[A-Z]{2,6}\.?|[A-Z0-9-]{2,}\.?$)'
    # literal form, ipv4 address (SMTP 4.1.3)
-    r'|^\[(25[0-5]|2[0-4]\d|[0-1]?\d?\d)'
-    r'(\.(25[0-5]|2[0-4]\d|[0-1]?\d?\d)){3}\]$'
+    r'|^\[(25[0-5]|2[0-4]\d|[0-1]?\d?\d)(\.(25[0-5]|2[0-4]\d|[0-1]?\d?\d)){3}\]$'


Same thing here.

I've added # fmt: on and # fmt: off, so these lines are back to being separate.

antoni-szych-rtbhouse · 2024-01-31T11:50:26Z

Mmm yeah, one last change please: can you make the default string quoting ' instead of ". That is the default in this project, and leaving it as-is will reduce the diff size considerably.

Sure, I've removed the string normalization commit (it was the last one) and the PR is smaller (+755 −433 vs +1,047 −730 previously).

I've also made a test and replaced all reasonable double quotes to single quotes in 231 lines. This would cause the diff to be +966 −648. Please let me know if case you would like me to commit this.

alecthomas

Thanks for doing this!

ds-cbo · 2024-01-31T15:02:44Z

This can be actually easily fixed both locally and for github blame: we just need to put the commit sha into the .git-blame-ignore-revs file. Note that it will have to be added after merging this PR, as we don't know the merge commit sha in advance :)

Now please don't forget to do that as well, now we have this:

antoni-szych-rtbhouse · 2024-01-31T15:47:44Z

Now please don't forget to do that as well, now we have this:

Done in #505, it will look like following (screenshot from my fork with this commit already merged):

As you can see, there are some lines annotated with the refactor commit, however these are only the new lines, which did not exist at all previously. However the important lines like 26-31, 36-40, 45-46, 51-53 show the pre-refactor annotation.

antoni-szych-rtbhouse added 2 commits December 14, 2023 13:39

move pytest settings to pyproject.toml

84c822c

add mypy settings to pyproject.toml

2356a35

ds-cbo suggested changes Dec 14, 2023

View reviewed changes

alecthomas reviewed Dec 15, 2023

View reviewed changes

antoni-szych-rtbhouse added 4 commits December 15, 2023 10:18

add black config, ci and tox entries

3941b18

add isort

2d75f28

isort

b1bc041

run isort

3dd5ea5

antoni-szych-rtbhouse added 2 commits December 15, 2023 13:40

don't use black for imports

703ce30

run black

10e984c

antoni-szych-rtbhouse force-pushed the linters branch from a69e414 to 10e984c Compare December 15, 2023 12:40

antoni-szych-rtbhouse requested review from ds-cbo and alecthomas December 15, 2023 13:13

antoni-szych-rtbhouse force-pushed the linters branch from 503f7da to 10e984c Compare January 31, 2024 10:47

alecthomas requested changes Jan 31, 2024

View reviewed changes

antoni-szych-rtbhouse force-pushed the linters branch from 9c3e0cd to 10e984c Compare January 31, 2024 11:34

revert reformatting of regexes

c850c11

antoni-szych-rtbhouse requested a review from alecthomas January 31, 2024 11:51

alecthomas approved these changes Jan 31, 2024

View reviewed changes

alecthomas merged commit c5189d2 into alecthomas:master Jan 31, 2024
9 checks passed

antoni-szych-rtbhouse deleted the linters branch January 31, 2024 15:38

antoni-szych-rtbhouse mentioned this pull request Jan 31, 2024

Ignore styling-only commit in git blame #505

Merged

Add linters configuration, reformat whole code #503

Add linters configuration, reformat whole code #503

Conversation

antoni-szych-rtbhouse commented Dec 14, 2023 • edited

ds-cbo commented Dec 14, 2023

ds-cbo commented Dec 14, 2023

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

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

antoni-szych-rtbhouse Dec 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoni-szych-rtbhouse Dec 15, 2023 • edited

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

alecthomas commented Dec 15, 2023 • edited

antoni-szych-rtbhouse commented Dec 15, 2023 • edited

antoni-szych-rtbhouse commented Jan 15, 2024

ds-cbo commented Jan 15, 2024

antoni-szych-rtbhouse commented Jan 30, 2024

alecthomas commented Jan 30, 2024

Choose a reason for hiding this comment

antoni-szych-rtbhouse Jan 31, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoni-szych-rtbhouse commented Jan 31, 2024 • edited

alecthomas left a comment

Choose a reason for hiding this comment

ds-cbo commented Jan 31, 2024

antoni-szych-rtbhouse commented Jan 31, 2024

antoni-szych-rtbhouse commented Dec 14, 2023 •

edited

antoni-szych-rtbhouse Dec 15, 2023 •

edited

antoni-szych-rtbhouse Dec 15, 2023 •

edited

alecthomas commented Dec 15, 2023 •

edited

antoni-szych-rtbhouse commented Dec 15, 2023 •

edited

antoni-szych-rtbhouse Jan 31, 2024 •

edited

antoni-szych-rtbhouse commented Jan 31, 2024 •

edited