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

implicit-str-concat: allow wrapping implicitly concatenated strings in parenthesis. #8552

Closed
yilei opened this issue Apr 7, 2023 · 3 comments · Fixed by #8590 or #9093
Closed

implicit-str-concat: allow wrapping implicitly concatenated strings in parenthesis. #8552

yilei opened this issue Apr 7, 2023 · 3 comments · Fixed by #8590 or #9093
Labels
Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Milestone

Comments

@yilei
Copy link
Contributor

yilei commented Apr 7, 2023

Current problem

Internally, we have enabled the --check-str-concat-over-line-jumps option for implicit-str-concat. But we also allow wrapping implicitly concatenated strings in parenthesis. This is done by our internal patch to Pylint ~3 years ago.

Example code:

a = [
    "aa bb"
    "cc dd",
]

It raises implicit-str-concat on the "aa bb" line, that's good to catch bugs of missing commas.

But for long strings that must be put on multiple lines, currently you need to use explicit str concatenations:

a = [
    "aaaaaaaa aaaaaaaa "
    + "aaaaaaaa aaaaaaaa ",
]

We think it should also be fine to use implicit str concatenations BUT wrapped inside parenthesis:

a = [
    ("aaaaaaaa aaaaaaaa "
     "aaaaaaaa aaaaaaaa "),
]

Desired solution

Introduce an opt-in option allow-parenthesized-str-concat. When enabled, allow the use of parenthesis.

Additional context

WDYT? If you agree, I'll send out our patch.

@yilei yilei added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Apr 7, 2023
@Pierre-Sassoulas
Copy link
Member

I think we should be compatible with what black does when a line is too long. I'm on mobile so I can't check and be 100% sure of it but I think black use parenthesis and implicit string concatenation so the proposed approach would work.

@yilei
Copy link
Contributor Author

yilei commented Apr 9, 2023

Yes Black (in current's preview style) wraps long strings (used in sequences) in parens, see playground.

Longs strings in func calls are the same today, but we have received feedback that it causes destructive diffs. So I'm proposing to not wrap (but also not using explicit str concatenation). See #3640 and psf/black#2188 (comment). So if this goes in to Black, I also plan to send a PR to Pylint to add an option allowing implicit str concat in func calls.

@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Apr 15, 2023
@Pierre-Sassoulas
Copy link
Member

Labeling this as Need PR because it's going to be easier to discuss the implication once a PR is tried and we can see the failing test cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
2 participants