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

[symilar] Migrate from getopt to argparse #9731

Merged
merged 5 commits into from
Aug 25, 2024
Merged

[symilar] Migrate from getopt to argparse #9731

merged 5 commits into from
Aug 25, 2024

Conversation

Pierre-Sassoulas
Copy link
Member

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

Follow-up to #9709, had to open a new PR because it's impossible to change the target branch in #9710 to reopen.

@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component duplicate-code Related to code duplication checker Maintenance Discussion or action around maintaining pylint or the dev workflow labels Jun 14, 2024
@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.3.0 milestone Jun 14, 2024
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the migrate-from-getopt-to-argparse-in-symilar branch from 7e17446 to 8e26d68 Compare June 14, 2024 07:25
@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as draft June 15, 2024 06:52
@Pierre-Sassoulas Pierre-Sassoulas mentioned this pull request Jun 16, 2024
Merged
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the migrate-from-getopt-to-argparse-in-symilar branch from 8e26d68 to 44e41d7 Compare June 17, 2024 07:51
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Pierre-Sassoulas
Copy link
Member Author

Thanks for the review @rogersheu

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the migrate-from-getopt-to-argparse-in-symilar branch from 7f40854 to 6d8bd0b Compare August 22, 2024 07:06
@codecov Codecov
Copy link

codecov bot commented Aug 22, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 95.80%. Comparing base (d048dcc) to head (ed1b787).
Report is 97 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #9731      +/-   ##
==========================================
- Coverage   95.80%   95.80%   -0.01%     
==========================================
  Files         174      174              
  Lines       18944    18918      -26     
==========================================
- Hits        18149    18124      -25     
+ Misses        795      794       -1
Files with missing lines Coverage Ξ”
pylint/checkers/symilar.py 96.31% <100.00%> (+0.01%) ⬆️
---- 🚨 Try these New Features:

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the migrate-from-getopt-to-argparse-in-symilar branch 2 times, most recently from 4bf4eeb to 689e144 Compare August 22, 2024 07:44
@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as ready for review August 22, 2024 07:47
@Pierre-Sassoulas Pierre-Sassoulas added the Needs review πŸ” Needs to be reviewed by one or multiple more persons label Aug 22, 2024
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
rogersheu
rogersheu reviewed Aug 25, 2024
View reviewed changes
tests/checkers/unittest_symilar.py
assert ex.value.code == 0
assert "similar lines in" in output.getvalue()


def test_bad_short_form_option() -> None:
def test_bad_short_form_option(capsys: CaptureFixture) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be out of scope of this PR, but could it be helpful to add a couple more unit tests?

  1. --i could be useful because it's ambiguous and argparse allows abbreviation (allow_abbrev defaults to True)
  2. I also hope a user wouldn't fall into this trap, but it might be relevant to have a test for if a user provides an argument accompaniment to one of the boolean flags (i.e., --ignore-comments 1 as you mentioned this in the fragments).

Copy link
Member Author

Choose a reason for hiding this comment

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

All of those test cases are going to fail with an elegant argparse message. We had a lot of unit tests because we were using getopt and doing too much ourselves (no other choices at time of implementation, as argparse was not builtin yet). I added test previously to check that I wasn't changing the behavior with the migration. I don't know if we should remove tests because we trust argparse now, maybe later (?)

Copy link
Contributor

@rogersheu rogersheu Aug 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see. Makes sense! Yeah, seeing as a lot of this error handling gets offloaded to argparse, I think it sounds like it would make sense to remove the tests when ready.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the migrate-from-getopt-to-argparse-in-symilar branch from 04c34f8 to 757d123 Compare August 25, 2024 06:02
@Pierre-Sassoulas
Copy link
Member Author

Thank you for the review @rogersheu, appreciated !

rogersheu
rogersheu reviewed Aug 25, 2024
View reviewed changes
pylint/checkers/symilar.py Outdated
Comment on lines 883 to 890
parser.add_argument("files", nargs="+")
parser.add_argument(
"-d", "--duplicates", type=int, default=DEFAULT_MIN_SIMILARITY_LINE
)
parser.add_argument("-i", "--ignore-comments", action="store_true")
parser.add_argument("--ignore-docstrings", action="store_true")
parser.add_argument("--ignore-imports", action="store_true")
parser.add_argument("--ignore-signatures", action="store_true")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Not sure under what circumstances users might end up seeing the parser's "usage" explanation, but if they might, help statements might be helpful here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch !

rogersheu
rogersheu approved these changes Aug 25, 2024
View reviewed changes
Copy link
Contributor

@rogersheu rogersheu left a comment

Choose a reason for hiding this comment

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

Left a tiny comment about help statements, but if the help for Run won't be seen, it's not necessary.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions 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 ed1b787

@Pierre-Sassoulas Pierre-Sassoulas merged commit 23de2d5 into main Aug 25, 2024
44 checks passed
@Pierre-Sassoulas Pierre-Sassoulas deleted the migrate-from-getopt-to-argparse-in-symilar branch August 25, 2024 21:24
Pierre-Sassoulas added a commit that referenced this pull request Aug 25, 2024
@Pierre-Sassoulas
Revert "[symilar] Migrate from getopt to argparse (#9731)"
558b191 
This reverts commit 23de2d5.
@Pierre-Sassoulas Pierre-Sassoulas removed the Needs review πŸ” Needs to be reviewed by one or multiple more persons label Aug 25, 2024
@Pierre-Sassoulas
Copy link
Member Author

Thank you again for the troubleshooting of the stupid bug I had, and the reviews @rogersheu

@Pierre-Sassoulas Pierre-Sassoulas mentioned this pull request Sep 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rogersheu rogersheu rogersheu approved these changes

@DanielNoord DanielNoord DanielNoord approved these changes

Assignees
No one assigned
Labels
duplicate-code Related to code duplication checker Enhancement ✨ Improvement to a component Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Milestone
3.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Pierre-Sassoulas @DanielNoord @rogersheu