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.

Sign up for GitHub

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

Jump to bottom

[pyreverse] Bugfix: strip "/" at the end of the file #8517

Merged
merged 9 commits into from
Apr 2, 2023
Merged

[pyreverse] Bugfix: strip "/" at the end of the file #8517

merged 9 commits into from
Apr 2, 2023

Conversation

qequ
Copy link
Contributor

@qequ qequ commented Mar 30, 2023

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

When calling pyreverse with an output filename like that ends with a "/" character like

pyreverse -o png -p packagename/ path_to_project/

the program crashes.

Added bugfix to strip those characters at the end of the output filename

Closes #8504

@qequ
Add rstripping for "/" characters
7112f89 
Signed-off-by: Alvaro Frias Garay <alvaro.frias@eclypsium.com>
@qequ qequ requested a review from DudeNr33 as a code owner March 30, 2023 14:30
@qequ qequ changed the title ugfix: strip "/" at the end of the file Bugfix: strip "/" at the end of the file Mar 30, 2023
@qequ qequ changed the title Bugfix: strip "/" at the end of the file [pyreverse] Bugfix: strip "/" at the end of the file Mar 30, 2023
@qequ
Add towncrier fragment
9b4b1d3 
Signed-off-by: Alvaro Frias Garay <alvaro.frias@eclypsium.com>
@qequ
Apply Precommit corrections
eea6603 
Signed-off-by: Alvaro Frias Garay <alvaro.frias@eclypsium.com>
@Pierre-Sassoulas Pierre-Sassoulas added Crash πŸ’₯ A bug that makes pylint crash backport maintenance/3.3.x pyreverse Related to pyreverse component labels Mar 30, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.17.2 milestone Mar 30, 2023
Pierre-Sassoulas
Pierre-Sassoulas reviewed Mar 30, 2023
View reviewed changes
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.

Looks pretty clean, thank you ! could you add a small test case too, please ?

@codecov
Copy link

codecov bot commented Mar 30, 2023 •
edited
Loading

Codecov Report

Merging #8517 (26a2884) into main (9f2de91) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8517      +/-   ##
==========================================
- Coverage   95.91%   95.90%   -0.01%     
==========================================
  Files         174      174              
  Lines       18362    18365       +3     
==========================================
+ Hits        17611    17613       +2     
- Misses        751      752       +1
Impacted Files Coverage Ξ”
pylint/pyreverse/writer.py 100.00% <100.00%> (ΓΈ)

... and 5 files with indirect coverage changes

@github-actions

This comment has been minimized.

Sign in to view
@DudeNr33
Copy link
Collaborator

Thanks for the fix!
Maybe we should expand this further and not only strip trailing slashes, but replace them entirely (with underscores, as for whitespaces).
The same crash would probably still happen if someone put a slash in the middle of the string, e. g. "on/off".

@qequ
Copy link
Contributor Author

qequ commented Mar 30, 2023

@DudeNr33 @Pierre-Sassoulas ok, gonna add tests and update the fix to replace all ocurrences of "/" in the string πŸ‘

qequ and others added 5 commits April 1, 2023 23:05
@qequ
Use replace rather than rstrip
b11c9b2 
Signed-off-by: Alvaro Frias Garay <alvaro.frias@eclypsium.com>
@qequ
Add tests
0f78615 
Signed-off-by: Alvaro Frias Garay <alvaro.frias@eclypsium.com>
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
a8fe00a 
for more information, see https://pre-commit.ci
@qequ
Correct precommit
ab737c3 
Signed-off-by: Alvaro Frias Garay <alvaro.frias@eclypsium.com>
@qequ
Merge branch 'bugfix-output-filename' of github.com:qequ/pylint into …
701ddaf 
…bugfix-output-filename
@github-actions

This comment has been minimized.

Sign in to view
Pierre-Sassoulas
Pierre-Sassoulas reviewed Apr 2, 2023
View reviewed changes
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.

Looks pretty good, I have two nitpicks

tests/pyreverse/test_writer.py Show resolved Hide resolved
tests/pyreverse/test_writer.py Outdated Show resolved Hide resolved
Pierre-Sassoulas
Pierre-Sassoulas reviewed Apr 2, 2023
View reviewed changes
tests/pyreverse/test_writer.py Outdated Show resolved Hide resolved
tests/pyreverse/test_writer.py Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas enabled auto-merge (squash) April 2, 2023 12:48
@Pierre-Sassoulas Pierre-Sassoulas merged commit 6ad17fb into pylint-dev:main Apr 2, 2023
@github-actions github-actions bot mentioned this pull request Apr 2, 2023
Merged
github-actions bot pushed a commit that referenced this pull request Apr 2, 2023
@qequ @github-actions
[pyreverse] Bugfix: strip "/" at the end of the file (#8517)
074f9b5 
Signed-off-by: Alvaro Frias Garay <alvaro.frias@eclypsium.com>
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
(cherry picked from commit 6ad17fb)
@github-actions
Copy link
Contributor

github-actions bot commented Apr 2, 2023

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

This comment was generated for commit 26a2884

@DudeNr33 DudeNr33 mentioned this pull request Apr 2, 2023
Merged
Pierre-Sassoulas pushed a commit that referenced this pull request Apr 2, 2023
@github-actions @qequ
[pyreverse] Bugfix: strip "/" at the end of the file (#8517) (#8528)
6dda042 
Signed-off-by: Alvaro Frias Garay <alvaro.frias@eclypsium.com>
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
(cherry picked from commit 6ad17fb)

Co-authored-by: Alvaro Frias <alvaro.frias@eclypsium.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

@DudeNr33 DudeNr33 Awaiting requested review from DudeNr33 DudeNr33 is a code owner

Assignees
No one assigned
Labels
Backported Crash πŸ’₯ A bug that makes pylint crash pyreverse Related to pyreverse component
Projects
None yet
Milestone
2.17.2
Development

Successfully merging this pull request may close these issues.

[pyreverse] Setting the project name with a "/" at the end crashes pyreverse
3 participants
@qequ @DudeNr33 @Pierre-Sassoulas