Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add linters configuration, reformat whole code #503

Merged
merged 9 commits into from
Jan 31, 2024

Conversation

antoni-szych-rtbhouse
Copy link
Contributor

@antoni-szych-rtbhouse antoni-szych-rtbhouse commented Dec 14, 2023

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
Copy link
Contributor

ds-cbo commented Dec 14, 2023

"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
Copy link
Contributor

ds-cbo commented Dec 14, 2023

"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.

from voluptuous.util import *
from voluptuous.error import *
from voluptuous.validators import *
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from voluptuous.error import * # isort: skip

made error go to the bottom of imports

object,
dict,
None,
typing.Callable,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing

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

Choose a reason for hiding this comment

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

move this comment back to the respective line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing.

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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

limiting the line length to default 88 did the trick

UrlInvalid,
raises,
validate,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure thing

extra=ALLOW_EXTRA))},
extra=ALLOW_EXTRA)
schema = Schema(
{"number": int, "follow": All(Self, Schema({"extra_number": int}, extra=ALLOW_EXTRA))}, extra=ALLOW_EXTRA
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

limiting the line length to default 88 did the trick

TrueInvalid,
TypeInvalid,
UrlInvalid,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@antoni-szych-rtbhouse antoni-szych-rtbhouse Dec 15, 2023

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@antoni-szych-rtbhouse antoni-szych-rtbhouse Dec 15, 2023

Choose a reason for hiding this comment

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

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.

@@ -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")
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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))
Copy link
Owner

Choose a reason for hiding this comment

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

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

TrueInvalid,
TypeInvalid,
UrlInvalid,
)
Copy link
Owner

Choose a reason for hiding this comment

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

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

)
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,}\.?$)"
Copy link
Owner

Choose a reason for hiding this comment

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

Please put these back on multiple lines for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alecthomas
Copy link
Owner

alecthomas commented Dec 15, 2023

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
Copy link
Contributor Author

antoni-szych-rtbhouse commented Dec 15, 2023

"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:
image

pycharm:
image

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

@antoni-szych-rtbhouse
Copy link
Contributor Author

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

@ds-cbo
Copy link
Contributor

ds-cbo commented Jan 15, 2024

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

@antoni-szych-rtbhouse
Copy link
Contributor Author

@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
Copy link
Owner

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.

@@ -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]+)*$"
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@antoni-szych-rtbhouse antoni-szych-rtbhouse Jan 31, 2024

Choose a reason for hiding this comment

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

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.

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}\]$'
Copy link
Owner

Choose a reason for hiding this comment

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

Same thing 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.

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

@antoni-szych-rtbhouse
Copy link
Contributor Author

antoni-szych-rtbhouse commented Jan 31, 2024

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.

Copy link
Owner

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@alecthomas alecthomas merged commit c5189d2 into alecthomas:master Jan 31, 2024
9 checks passed
@ds-cbo
Copy link
Contributor

ds-cbo commented Jan 31, 2024

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:

image

@antoni-szych-rtbhouse
Copy link
Contributor Author

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):

image

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants