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

Allow parenthesized implicitly concatenated strings inside calls, to be more compatible with Black. #8590

Merged
merged 8 commits into from
Jun 18, 2023

Conversation

yilei
Copy link
Contributor

@yilei yilei commented Apr 18, 2023

Type of Changes

Type
πŸ› Bug fix
βœ“ ✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Description

For discussion in #8552.

Closes #8552

For reference, here is Black's preview style results with relevant usages: playground

Alternatively, we could add another option (default to keep the existing behavior) to silence parenthesized implicit-str-concat.

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #8590 (a315e0c) into main (92485a3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8590   +/-   ##
=======================================
  Coverage   95.84%   95.84%           
=======================================
  Files         173      173           
  Lines       18466    18492   +26     
=======================================
+ Hits        17698    17724   +26     
  Misses        768      768           
Impacted Files Coverage Ξ”
pylint/checkers/strings.py 94.14% <100.00%> (+0.37%) ⬆️

@github-actions

This comment has been minimized.

@DanielNoord
Copy link
Collaborator

@yilei I guess nobody has time to review this yet, but as a start could you look at the coverage issue? You seem to be missing a line.

@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Apr 21, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.0.0b1 milestone Apr 21, 2023
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Sorry to react only after I read the functional tests I was not understanding things properly before. I think I thought and started to create test case about this earlier but I lost the change when my hard drive crashed because I can't find the branch anymore. What I wanted to do at some point was to check problem on a single line even inside multiline:

print(
    "Lorem ipsum dolor sit amet, ""consectetur adipiscing elit,"  # [implicit-str-concat] (notice the "" before 'consectetur')
    "Lorem ipsum dolor sit amet, consectetur adipiscing elit,"
    " sed do eiusmod tempor incididunt ut labore et dolore "
    "magna aliqua. Ut enim ad minim veniam, quis nostrud "
    "exercitation ullamco laboris nisi ut aliquip ex ea "
)

I.e. if the string concat happens on a single line then raise implicit-str-concat else do not raise. But the approach from the checker was to be changed completely for that to work (use token I think?). Let me know what you think :)

@yilei
Copy link
Contributor Author

yilei commented Apr 22, 2023

@DanielNoord The coverage issue should be addressed. The missed line was actually dead code that can't be reached.

@Pierre-Sassoulas Thanks for taking a look. If they are on the same line, implicit-str-concat is always raised even in a multi line case. I added your example as a test case. The checker is using tokens and checking token's line numbers. Or did I misunderstand something?

@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0.0b1, 3.0.0a7 May 16, 2023
@Pierre-Sassoulas Pierre-Sassoulas added the Needs review πŸ” Needs to be reviewed by one or multiple more persons label May 16, 2023
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

LGTM, thank you !

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Thanks! I think this is ready to merge. I found some minor typos.

Finally, it would be worth removing the reference to this PR added in #8649 and just show beneath the example there how to rewrite it correctly (as black will do it, soon).

pylint/checkers/strings.py Outdated Show resolved Hide resolved
pylint/checkers/strings.py Outdated Show resolved Hide resolved
pylint/checkers/strings.py Outdated Show resolved Hide resolved
@jacobtylerwalls jacobtylerwalls removed the Needs review πŸ” Needs to be reviewed by one or multiple more persons label Jun 18, 2023
@jacobtylerwalls jacobtylerwalls dismissed stale reviews from Pierre-Sassoulas and themself via e2ee5a2 June 18, 2023 15:28
@jacobtylerwalls jacobtylerwalls changed the title Try allowing parenthesized implicitly concatenated strings, to be more compatible with Black. Allow parenthesized implicitly concatenated strings inside calls, to be more compatible with Black. Jun 18, 2023
@jacobtylerwalls jacobtylerwalls enabled auto-merge (squash) June 18, 2023 15:32
@jacobtylerwalls jacobtylerwalls merged commit 81917bc into pylint-dev:main Jun 18, 2023
40 checks passed
@github-actions
Copy link
Contributor

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit a315e0c

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

Successfully merging this pull request may close these issues.

implicit-str-concat: allow wrapping implicitly concatenated strings in parenthesis.
4 participants