fixing e501 for tests/commands#2688
fixing e501 for tests/commands#2688JohananOppongAmoateng wants to merge 7 commits intobeeware:mainfrom
Conversation
f86ad08 to
e637047
Compare
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for those updates - looks like more good progress towards clean Ruff output.
I've flagged a few places where the formatting is valid, but needs an update to be consistent with our internal style. I've also flagged a couple of places where the style choices are a bit odd. If those places are the result of automated update from Ruff, then we'll live with the output of that tool - but if they're manually applied choices, in most cases, the older syntax is preferred.
| "\n stderr: '\nfatal: could not clone repository" | ||
| " 'https://example.com' \n'", |
There was a problem hiding this comment.
As noted in the past PR - when we've got a string as one argument of many, it's preferable to use bracket notation to highlight that this is one string argument, not two:
| "\n stderr: '\nfatal: could not clone repository" | |
| " 'https://example.com' \n'", | |
| ( | |
| "\n stderr: '\nfatal: could not clone repository" | |
| " 'https://example.com' \n'" | |
| ), |
| "\n stderr: '\nfatal: Could not read from remote repository.\n\nPlease" | ||
| " make sure you have the correct access rights\nand the repository" | ||
| " exists. \n'", | ||
| "Could not read from remote repository.\n\nPlease make sure " | ||
| "you have the correct access rights\nand the repository exists.", |
There was a problem hiding this comment.
As above - these two should be bracket-wrapped
| "\n stderr: '\nfatal: unable to access 'https://example.com/': " | ||
| "OpenSSL/3.2.2: error:0A000438:SSL routines::tlsv1 alert internal error'", | ||
| "Unable to access 'https://example.com/': OpenSSL/3.2.2: " | ||
| "error:0A000438:SSL routines::tlsv1 alert internal error.", |
There was a problem hiding this comment.
As above - these two should be bracket-wrapped
| f"The directory {bundle_path.relative_to(base_path)} already exists; overwrite" | ||
| " [y/N]? " |
There was a problem hiding this comment.
Examples like this one are a bit of a judgement call as to whether extra parentheses are needed, as it's a list with one item; I'd be inclined to they should, to reinforce that it's a single prompt in a list.
(Similarly for the 2 other equivalent changes in this file)
| filename=tmp_path | ||
| / "data/stub/986428ef9d5a1852fc15d4367f19aa328ad530686056e9d83cdde03407c0bceb/My-Stub.zip", | ||
| / "data/stub/986428ef9d5a1852fc15d4367f19aa328ad530686056e9d83cdde03407c0bceb/My-Stub.zip", # noqa: E501 |
There was a problem hiding this comment.
Preference here would be (a) to use brackets to clarify the scope of the filename argument, and (b) to break the string in more places to avoid the need for the noqa. Github won't let me code suggest here, but something like:
filename=(
tmp_path
/ "data/stub"
/ "986428ef9d5a1852fc15d4367f19aa328ad530686056e9d83cdde03407c0bceb"
/ "My-Stub.zip"
),
tests/commands/upgrade/test_call.py
Outdated
| assert capsys.readouterr().out == ( | ||
| "\n" | ||
| assert ( | ||
| capsys.readouterr().out == "\n" |
There was a problem hiding this comment.
This is an odd change - the brackets are the exact opposite of what I would have expected. The reasoning is the same as for other bracket usage - it's not intuitive to tell which statements are the start of a string, and which the start of a new argument. The previous formatting made this clear - copses.readouterr().out = (...some multiline string...); the new formatting is ambiguous.
|
|
||
| def test_test_dependencies_without_requires(): | ||
| "If the global config doesn't specify test requirements, test dependencies are used as is" | ||
| """If the global config doesn't specify test requirements, test " "dependencies are |
There was a problem hiding this comment.
A stray pair of quotes, probably from a line break that existed at one point:
| """If the global config doesn't specify test requirements, test " "dependencies are | |
| """If the global config doesn't specify test requirements, test dependencies are |
| ( | ||
| "There is nothing wrong with your television set.\n" | ||
| "Do not attempt to adjust the picture." | ||
| ), |
There was a problem hiding this comment.
+1 to this change - this is the preferred format for multi-line strings.
| "Wait message... started\nWait message... finished\n" | ||
| assert ( | ||
| capsys.readouterr().out == "Wait message... started\nWait message... finished\n" | ||
| ) |
There was a problem hiding this comment.
This is a bit of a value judgement - if an automated tool is doing this, then fine; but otherwise the older format would be preferred.
| "... still waiting\n... still waiting\n... still waiting\n" | ||
| assert ( | ||
| capsys.readouterr().out | ||
| == "... still waiting\n... still waiting\n... still waiting\n" |
There was a problem hiding this comment.
Again - the older format would be preferred unless this is a tool automated change.
This pr is a continuation of fixes for the e501 issues in #2383
PR Checklist: