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

Wrap long lines to comply with flake8 E501 #4985

Merged
merged 24 commits into from
Jan 11, 2022

Conversation

stinodego
Copy link
Contributor

@stinodego stinodego commented Jan 5, 2022

Resolves: #4776

Hi! This is my first PR for poetry; please let me know how I can improve my work.

Because of the nature of the issue being fixed (linting rules), this PR touches a lot of files. There are no functional changes, however, so I figured it would be OK for a single PR. Let me know if you would like me to break it up into smaller PRs (an option would be tests first, then src, for example).

Changes

(updated 2020-01-08)

  • Updated .flake8 to remove the E501 error (line too long) from the list of ignored warnings.
  • Added poetry to isort namespace packages - see discussion below for the reason why.
  • Explicitly ignored the E501 error for some long test function names and the occasional URL/path/hash.
  • Apply black --experimental-string-processing to handle long strings. Added this functionality to pre-commit.
  • Manually wrap some strings that black did not format (notably strings without spaces).
  • Manually wrap multi-line strings.
  • Manually wrap some docstrings and in-line comments.
  • Fixed a typo in src/poetry/console/commands/source/add.py: "already exits" -> "already exists"
  • Fixed a few more rare typos in docs/comments.

String wrapping approach

(updated 2020-01-07)

I try to be consistent with the formatting guidelines of black, and the style of the rest of the code base.

For long regular strings that are not handled by black, I wrap them using implicit string concatenation, with leading space and if it's an f-string, an 'f' only at the lines where it's required:

# Original
long_string = f"This string is way too long for a code base with a maximum character limit of {88}."

# Wrapped
long_string = (
    "This string is way too long for a code base"
    f" with a maximum character limit of {88}."
)

For multi-line strings, I use backslash breaks with leading space:

# Original
long_string = """\
This string is way too long for a code base with a maximum character limit of 88.
"""

# Wrapped
long_string = """\
This string is way too long for a code base\
 with a maximum character limit of 88.
"""

@finswimmer
Copy link
Member

Hello @stinodego,

thanks a lot for the work you've already done 👍

It took a while but I've found out what's going on with isort. It's due to the change in src/poetry/__init__.py. See PyCQA/isort#1879

To fix it we can put this in the isort config section in the pyproject.toml:

namespace_packages = ["poetry"]

Regarding the string wrappings:

black has an experimental flag --experimental-string-processing (See Release Notes) This is already used by larger projects like django, pandas and sqlalchemy and will become default in a future version (see psf/black#2188). I use this in my project as well. I think we can/should add this to Poetry as well.

Because black has some different opinion about the wrapping then how you have done it, I would ask you to revert your string wrapping changes first and run black with the above flag. 🙏

fin swimmer

@stinodego
Copy link
Contributor Author

Hi @finswimmer ! Thanks for your feedback, I will implement the changes you requested.

And kudos for finding out what caused that isort behaviour; definitely an obscure corner case!

Will redo this PR this weekend.

@stinodego stinodego force-pushed the 4776-line-length branch 2 times, most recently from 0585767 to 8fa7926 Compare January 7, 2022 19:43
@stinodego
Copy link
Contributor Author

PR has been re-done! Looking forward to your feedback.

PS: I see one CI check failing; I don't believe this is something I can fix?

Copy link
Member

@finswimmer finswimmer left a comment

Choose a reason for hiding this comment

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

Looks quite good to me. Thanks a lot 👍

Can you please add the --experimental-string-processing flag to the list of args for black in the .pre-commit-config.yaml?

src/poetry/__init__.py Outdated Show resolved Hide resolved
@stinodego
Copy link
Contributor Author

stinodego commented Jan 8, 2022

Can you please add the --experimental-string-processing flag to the list of args for black in the .pre-commit-config.yaml?

Done! Had to exclude the get-poetry.py and install-poetry.py scripts, because those do not comply. I borrowed the exclude regex from the pyupgrade hook config.

There's a few places where the black string processing had a different opinion than vanilla black and me; so you'll see a few code changes in the latest commit too.

EDIT: Actually, would it not be better to include this setting in the pyproject.toml? The other settings are there too, and that way, my editor picks it up for autoformatting.
I am adding a new commit with this adjustment - just revert it if you do prefer to have the configuration in pre-commit.

@stinodego stinodego requested a review from finswimmer January 8, 2022 12:37
@finswimmer
Copy link
Member

Actually, would it not be better to include this setting in the pyproject.toml?

This is a very good idea 👍

Had to exclude the get-poetry.py and install-poetry.py scripts, because those do not comply.

Hm, I don't see any reasons why we should exclude them. What problems have you faced?

@stinodego
Copy link
Contributor Author

stinodego commented Jan 8, 2022

Had to exclude the get-poetry.py and install-poetry.py scripts, because those do not comply.

Hm, I don't see any reasons why we should exclude them. What problems have you faced?

These scripts do not follow the experimental string formatting rules of black. This was no problem before, because the scripts DO comply to the standard black formatting rules. Now, due to the change in black settings, pre-commit will fail if we do not exclude these scripts.

I don't mind formatting these scripts too, but I understood that these scripts were 'frozen' and not allowed to be changed.

So we should exclude them either in pyproject.toml (which I have done now) or in the pre-commit config.

@finswimmer
Copy link
Member

I don't mind formatting these scripts too, but I understood that these scripts were 'frozen' and not allowed to be changed.

It's fine to change these scripts if needed. Here we have just changes of the format. We should not exclude these scripts from black.

We have fixed the failing check in the meantime (Well, actually we drop it). You will need to rebase your branch onto master to pass all checks.

@stinodego
Copy link
Contributor Author

stinodego commented Jan 8, 2022

All right, rebased and done!

Comment on lines 490 to 491
"\"import sys; print('.'.join([str(s) for s in"
' sys.version_info[:3]]))"',
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should keep the original format here for better readability. What do you think? (same for the lines 730 and 805, 859)

This can be achieved by #fmt: off/on (https://stackoverflow.com/a/58584557/9750706)

Copy link
Contributor Author

@stinodego stinodego Jan 8, 2022

Choose a reason for hiding this comment

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

In this case, because the exact same string is repeated 4 times in this class, I'd suggest defining this as a string constant in the class (next to ENVS_FILE) called something like VERSION_SCRIPT.

Then we won't have to wrap it, and won't have to fiddle with black formatting.

(did this in the most recent commit, let me know what you think)

Copy link
Member

Choose a reason for hiding this comment

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

Good idea 👍 I saw there is already a GET_PYTHON_VERSION at the top of the file. I think we can use this one.

Copy link
Contributor Author

@stinodego stinodego Jan 10, 2022

Choose a reason for hiding this comment

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

Even better! Adjusted the last commit to use the existing constant.

...or now that I think about it; maybe there is a reason to use a 'one-liner' version here, because it looks like command line arguments? Tests still pass though.

EDIT: I tested some things in a Notebook and I don't think the multi-line version will work, but maybe I am mistaken. We could just add this constant below the GET_PYTHON_VERSION constant as a GET_PYTHON_VERSION_ONELINER and use that instead. Avoid the trouble :) (adjusted the commit to this idea)

tests/installation/test_executor.py Outdated Show resolved Hide resolved
Copy link
Member

@finswimmer finswimmer left a comment

Choose a reason for hiding this comment

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

Beautiful 👍 🥇 🚀

@finswimmer finswimmer merged commit ecb030e into python-poetry:master Jan 11, 2022
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code Quality: Additional lints in this repository (help wanted!)
2 participants