Skip to content

Make multiline ISC explicit#2266

Closed
Avasam wants to merge 6 commits intomhammond:mainfrom
Avasam:multiline-ISC
Closed

Make multiline ISC explicit#2266
Avasam wants to merge 6 commits intomhammond:mainfrom
Avasam:multiline-ISC

Conversation

@Avasam
Copy link
Copy Markdown
Collaborator

@Avasam Avasam commented May 28, 2024

Follow-up to #2225
adodbapi's version of this PR: #2265

Make multiline Implicit String Concatenation explicit to avoid mixing up parameters and concatenation.
Also fixed missing closing parenthesis on win32com.client.build.MapEntry's string representation since I was already modifying that line.

Found using ruff check --select=ISC002 --exclude=adodbapi.

ruff.toml Outdated
extra-standard-library = ["setuptools"]

[lint.flake8-implicit-str-concat]
allow-multiline = false
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This doesn't add any new check, it just allows ISC002 to be checked.

@Avasam Avasam requested a review from mhammond May 28, 2024 22:09
@mhammond
Copy link
Copy Markdown
Owner

I don't really like this - the fact strings like this are handled is well documented as a feature of strings.

@Avasam
Copy link
Copy Markdown
Collaborator Author

Avasam commented May 29, 2024

It is well documented, but it's also easily a source of potential errors when writing (easy to forget a comma, resulting in an accidental ISC), and misinterpretation at a quick glance.

Although less so with type-checkers in place, since parameter count and tuple length are checked but only in some cases (which'll grow and improve with time and annotations).

That being said, that's just my opinion, , and this PR a suggestion. If you do want to keep multiline implicit string concatenation, then I'll comment the config as such.

@mhammond
Copy link
Copy Markdown
Owner

I don't think we need to be 100% consistent here - eg, I wouldn't ask someone who did concatenate a string split across 2 lines to change it, just like I don't think it's worth asking/insisting someone who split strings to change it to a concat. I don't want to get too dogmatic here, and now that we've covered all the low-hanging fruit and removed all the likely footguns, I fear we are starting to move into that territory.

@Avasam Avasam mentioned this pull request May 29, 2024
@Avasam
Copy link
Copy Markdown
Collaborator Author

Avasam commented May 29, 2024

Closing in favor of #2273 !

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.

2 participants