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

Arguments #101

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ Example, preview for update target list by ``pip list --outdated`` format, with
$ pip-review --auto --preview
... <same above and pip install output>

Example, only preview for update target list:
Example, preview a table of the update target list, which is a combination of the interactive and preview formats:
Copy link
Owner

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.

Copy link
Owner

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.


.. code:: console

Expand Down
120 changes: 101 additions & 19 deletions pip_review/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,30 +69,62 @@ def parse_args():
description=description,
epilog=EPILOG+version_epilog(),
)

# ----------------------------
Copy link
Owner

Choose a reason for hiding this comment

The 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
Copy link
Owner

Choose a reason for hiding this comment

The 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; --verbose does not mean "show me the wordy format" but it means "show all output with logger.debug in addition to all output with logger.info". It so happens that pip-review does not have such output anymore; all former calls to logger.debug were in Vincent Driessen's custom logic to compute the outdated packages, which fell out of the codebase when we transitioned to a thin wrapper around pip list --outdated in version 1.0.0. It is only now that I realize that the --verbose flag doesn't make any difference anymore because of this!

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 --raw or --preview. You could further clarify this by changing the header from just example: to default format:.

Oh, and one more thing: it is conventional to use double triple quotes for comments, so """ instead of '''.


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
Copy link
Owner

Choose a reason for hiding this comment

The 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
Copy link
Owner

Choose a reason for hiding this comment

The 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 --raw and its comment. That would group all the formats together.


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()


Expand Down Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

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

Here, you're changing the meaning of the --verbose flag. Please don't change the meaning of already released flags!

Comment on lines +315 to +318
Copy link
Owner

Choose a reason for hiding this comment

The 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
Copy link
Owner

Choose a reason for hiding this comment

The 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 on the next line.


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
Copy link
Owner

Choose a reason for hiding this comment

The 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:

  • pip-review
    • report: wordy
    • updates: none
  • pip-review --auto
    • report: no output
    • updates: all
  • pip-review --raw
    • report: raw
    • updates: none
  • pip-review --interactive
    • report: wordy (combined with selection)
    • updates: interactive selection
  • --verbose combined with any of the above: would give diagnostic output, if our code would actually use that

Behavior on develop (after changes by @kunif):

  • pip-review
    • report: wordy
    • updates: none
  • pip-review --preview-only
    • report: table
    • updates: none
  • pip-review --preview
    • report: table
    • updates: none, but repeats all available updates in wordy format
  • pip-review --preview --auto
    • report: table
    • updates: all
  • pip-review --preview --raw
    • report: table
    • updates: none, but also raw report shown
  • pip-review --preview --interactive
    • report: table
    • updates: interactive selection (in wordy format)
  • pip-review --auto
    • report: no output
    • updates: all
  • pip-review --raw
    • report: raw
    • updates: none
  • pip-review --interactive
    • report: wordy (combined with selection)
    • updates: interactive selection
  • --verbose combined with any of the above: would give diagnostic output, if our code would actually use that

Behavior that I think would make most sense (nearly the same but no --preview-only and no --preview --raw):

  • pip-review
    • report: wordy
    • updates: none
  • pip-review --preview
    • report: table
    • updates: none (as --preview-only on current develop)
  • pip-review --preview --auto
    • report: table
    • updates: all
  • pip-review --preview --interactive
    • report: table
    • updates: interactive selection (in wordy format)
  • pip-review --auto
    • report: no output
    • updates: all
  • pip-review --raw
    • report: raw
    • updates: none
  • pip-review --interactive
    • report: wordy (combined with selection)
    • updates: interactive selection
  • --verbose combined with any of the above: would give diagnostic output, if our code would actually use that

Behavior after your changes in d05f518..d0c7792:

  • pip-review
    • nothing happens, this is a breaking change!
  • pip-review --verbose
    • report: wordy
    • updates: none
    • also enables diagnostic input as before
  • pip-review --verbose --raw
    • report: wordy
    • updates: none, but also raw report shown
  • pip-review --verbose --interactive
    • report: wordy
    • updates: interactive selection (each available update mentioned twice)
  • pip-review --verbose --interactive --auto
    • report: wordy
    • first round of updates: interactive selection (each available update mentioned twice)
    • second round of updates: all (based on situation before previous round)
  • pip-review --verbose --interactive --preview
    • first report: wordy
    • updates: interactive selection (each available update mentioned twice)
    • second report: table (based on data before interactive update)
  • pip-review --verbose --interactive --preview --visual
    • first report: wordy
    • first round of updates: interactive selection (each available update mentioned twice)
    • second report: table (based on data before interactive update)
    • second round of updates:
      • interactive selection (still based on data before previous interactive update)
      • selection displayed in a table
      • confirmation
  • pip-review --verbose --interactive --visual
    • report: wordy
    • first round of updates: interactive selection (each available update mentioned twice)
    • second round of updates:
      • interactive selection (based on data before previous interactive update)
      • selection displayed in a table
      • confirmation
  • pip-review --verbose --auto
    • report: wordy
    • updates: all
  • pip-review --verbose --preview
    • first report: wordy
    • second report: table
  • pip-review --verbose --preview --visual
    • first report: wordy
    • second report: table
    • updates:
      • interactive selection (each available update mentioned three times)
      • selection displayed in a table
      • confirmation
  • pip-review --verbose --visual
    • report: wordy
    • updates:
      • interactive selection (each available update mentioned twice)
      • selection displayed in a table
      • confirmation
  • pip-review --raw
    • report: raw
    • updates: none
  • pip-review --interactive
    • report: wordy (combined with selection)
    • updates: interactive selection
  • pip-review --interactive --auto
    • report: wordy (combined with selection)
    • first round of updates: interactive selection
    • second round of updates: all (based on situation before previous round)
  • pip-review --interactive --preview
    • first report: wordy (combined with selection)
    • updates: interactive selection
    • second report: table (based on data before interactive update)
  • pip-review --interactive --preview --visual
    • first report: wordy (combined with selection)
    • first round of updates: interactive selection
    • second report: table (based on data before interactive update)
    • second round of updates:
      • interactive selection (still based on data before previous interactive update)
      • selection displayed in a table
      • confirmation
  • pip-review --auto
    • report: no output
    • updates: all
  • pip-review --preview
    • report: table
    • updates: none
  • pip-review --preview --visual
    • report: table
    • updates:
      • interactive selection
      • selection displayed in a table
      • confirmation
  • pip-review --visual
    • updates:
      • interactive selection
      • selection displayed in a table
      • confirmation


if __name__ == '__main__':
try:
main()
except KeyboardInterrupt:
sys.stdout.write('\nAborted\n')
sys.exit(0)
sys.exit(0)
Copy link
Owner

Choose a reason for hiding this comment

The 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.