-
Notifications
You must be signed in to change notification settings - Fork 36
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
Arguments #101
base: develop
Are you sure you want to change the base?
Arguments #101
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,30 +69,62 @@ def parse_args(): | |
description=description, | ||
epilog=EPILOG+version_epilog(), | ||
) | ||
|
||
# ---------------------------- | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm in favor of adding a blank line here. The ruler is maybe not so necessary. If you want to keep the ruler, please give it the same indentation as the surrounding code. |
||
|
||
parser.add_argument( | ||
'--verbose', '-v', action='store_true', default=False, | ||
help='Show more output') | ||
|
||
''' example: | ||
ezdxf==1.0.2 is available (you have 0.8.0) | ||
pip==23.0.1 is available (you have 22.0.4) | ||
setuptools==67.3.2 is available (you have 58.1.0) | ||
''' | ||
Comment on lines
+79
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a nice touch, makes the meaning of the arguments more tangible for the programmer. This comment is in the wrong place, though; Anyway, the comment you wrote here would probably be more appropriate on line 73, where you put the ruler. This is because the wordy format is the default format, which appears if you don't pass Oh, and one more thing: it is conventional to use double triple quotes for comments, so |
||
|
||
parser.add_argument( | ||
'--raw', '-r', action='store_true', default=False, | ||
help='Print raw lines (suitable for passing to pip install)') | ||
|
||
''' example: | ||
ezdxf==1.0.2 | ||
pip==23.0.1 | ||
setuptools==67.3.2 | ||
''' | ||
Comment on lines
+89
to
+93
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is correct here. 👍 |
||
|
||
parser.add_argument( | ||
'--interactive', '-i', action='store_true', default=False, | ||
help='Ask interactively to install updates') | ||
|
||
parser.add_argument( | ||
'--auto', '-a', action='store_true', default=False, | ||
help='Automatically install every update found') | ||
|
||
parser.add_argument( | ||
'--continue-on-fail', '-C', action='store_true', default=False, | ||
help='Continue with other installs when one fails') | ||
|
||
parser.add_argument( | ||
'--freeze-outdated-packages', action='store_true', default=False, | ||
help='Freeze all outdated packages to "requirements.txt" before upgrading them') | ||
|
||
parser.add_argument( | ||
'--preview', '-p', action='store_true', default=False, | ||
help='Preview update target list before execution') | ||
|
||
''' exmple: | ||
Package Version Latest Type | ||
------------------------------- | ||
ezdxf 0.8.0 1.0.2 wheel | ||
pip 22.0.4 23.0.1 wheel | ||
setuptools 58.1.0 67.3.2 wheel | ||
------------------------------- | ||
''' | ||
Comment on lines
+115
to
+122
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again correct, and nice for consistency. Would be even better if you move the argument and this comment up, so they come directly after |
||
|
||
parser.add_argument( | ||
'--preview-only', '-P', action='store_true', default=False, | ||
help='Preview only') | ||
'--visual', '-s', action='store_true', default=False, | ||
help='Interactively select the packages for update, see a visual report and do the updates') | ||
|
||
return parser.parse_known_args() | ||
|
||
|
||
|
@@ -276,42 +308,92 @@ def main(): | |
list_args = filter_forwards(forwarded, INSTALL_ONLY) | ||
install_args = filter_forwards(forwarded, LIST_ONLY) | ||
logger = setup_logging(args.verbose) | ||
outdated = get_outdated_packages(list_args) | ||
|
||
# --verbose | ||
if args.verbose: | ||
for pkg in outdated: | ||
logger.info('{0}=={1} is available (you have {2})'.format( | ||
pkg['name'], pkg['latest_version'], pkg['version'] | ||
)) | ||
Comment on lines
+314
to
+318
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, you're changing the meaning of the
Comment on lines
+315
to
+318
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, this same snippet of code now appears in three places. Keep it DRY: Don't Repeat Yourself! |
||
|
||
# --raw | ||
# --interactive | ||
Comment on lines
+320
to
+321
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These comments don't really add information; it already says |
||
|
||
if args.raw and args.interactive: | ||
raise SystemExit('--raw and --interactive cannot be used together') | ||
|
||
outdated = get_outdated_packages(list_args) | ||
if not outdated and not args.raw: | ||
logger.info('Everything up-to-date') | ||
return | ||
if args.preview or args.preview_only: | ||
logger.info(format_table(extract_table(outdated))) | ||
if args.preview_only: | ||
return | ||
if args.auto: | ||
update_packages(outdated, install_args, args.continue_on_fail, args.freeze_outdated_packages) | ||
return | ||
|
||
# --raw | ||
if args.raw: | ||
for pkg in outdated: | ||
logger.info('{0}=={1}'.format(pkg['name'], pkg['latest_version'])) | ||
return | ||
|
||
selected = [] | ||
for pkg in outdated: | ||
logger.info('{0}=={1} is available (you have {2})'.format( | ||
pkg['name'], pkg['latest_version'], pkg['version'] | ||
)) | ||
if args.interactive: | ||
# --interactive | ||
if args.interactive: | ||
selected = [] | ||
|
||
for pkg in outdated: | ||
logger.info('{0}=={1} is available (you have {2})'.format( | ||
pkg['name'], pkg['latest_version'], pkg['version'] | ||
)) | ||
|
||
answer = ask_to_install() | ||
|
||
if answer in ['y', 'a']: | ||
selected.append(pkg) | ||
if selected: | ||
update_packages(selected, install_args, args.continue_on_fail, args.freeze_outdated_packages) | ||
|
||
if selected: | ||
update_packages(selected, install_args, args.continue_on_fail, args.freeze_outdated_packages) | ||
|
||
# --auto | ||
if args.auto: | ||
update_packages(outdated, install_args, args.continue_on_fail, args.freeze_outdated_packages) | ||
return | ||
|
||
# --preview | ||
if args.preview: | ||
logger.info(format_table(extract_table(outdated))) | ||
|
||
# --preview-only | ||
if args.visual: | ||
selected = [] | ||
|
||
for pkg in outdated: | ||
logger.info('{0}=={1} is available (you have {2})'.format( | ||
pkg['name'], pkg['latest_version'], pkg['version'] | ||
)) | ||
|
||
answer = ask_to_install() | ||
|
||
if answer in ['y', 'a']: | ||
selected.append(pkg) | ||
|
||
if not selected: | ||
logger.info('No packages selected for update / upgrade') | ||
return | ||
|
||
if selected: | ||
logger.info('Selected packages for update / upgrade are') | ||
logger.info(format_table(extract_table(selected))) | ||
|
||
answer = ask_to_install() | ||
|
||
if answer in ['y', 'a']: | ||
update_packages(selected, install_args, args.continue_on_fail, args.freeze_outdated_packages) | ||
|
||
else: | ||
logger.info('You chose to quit the update process') | ||
|
||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are way too many changes on the past 80 lines. Firstly, because you did not need to make so many changes, secondly, because some of the changes are disruptive for users, and thirdly, because you lost track of all the possible interactions between the options. Let us compare pip-review's behavior in the past to what these changes made of it. Behavior in all published releases so far (up to 1.3.0), by order of precedence of options:
Behavior on
Behavior that I think would make most sense (nearly the same but no
Behavior after your changes in d05f518..d0c7792:
|
||
|
||
if __name__ == '__main__': | ||
try: | ||
main() | ||
except KeyboardInterrupt: | ||
sys.stdout.write('\nAborted\n') | ||
sys.exit(0) | ||
sys.exit(0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't strip newlines from the ends of files! Some tools rely on those newlines being there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a combination, is it? I think the next example is just the preview (table) format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, note that it is still saying
--preview-only
on line 64.