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

fix(run): simplify and fix run arg parsing #10067

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abn
Copy link
Member

@abn abn commented Jan 17, 2025

Resolves: #10051

Summary by Sourcery

Simplify the parsing of arguments for the "run" command and fix a bug related to passing arguments after the command name.

Bug Fixes:

  • Fix a bug where arguments passed after the "run" command were not being handled correctly.

Enhancements:

  • Simplify the logic for parsing arguments in the "run" command.

@abn abn added the area/cli Related to the command line label Jan 17, 2025
@abn abn requested a review from a team January 17, 2025 14:26
Copy link

sourcery-ai bot commented Jan 17, 2025

Reviewer's Guide by Sourcery

This PR simplifies the argument parsing for the run command by ensuring that arguments and options intended for the executed script are correctly passed. It fixes a bug where options passed before the script name were not being handled correctly and simplifies the logic for separating command options from script arguments.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change Details Files
Refactored argument parsing for the run command.
  • Replaced the _configure_io method with _configure_run_command to specifically handle the run command logic.
  • Removed the custom RunArgvInput class and related logic.
  • Simplified the argument parsing by using a single ArgvInput instance and strategically inserting -- to separate Poetry's options from the script's arguments.
  • Added logic to handle options passed before the run command and ensure they are applied correctly.
  • Improved the handling of arguments passed after the script name to ensure they are passed to the script correctly.
src/poetry/console/application.py
Removed the RunArgvInput class.
  • Deleted the entire RunArgvInput class as it is no longer needed with the simplified argument parsing logic.
src/poetry/console/io/inputs/run_argv_input.py
Added tests to cover the new argument parsing behavior.
  • Added tests to verify that arguments passed after the run command but before the script name are handled correctly.
  • Added tests to confirm that options passed before the run command are correctly applied and do not affect the script execution.
  • Added tests to ensure that the -V option, when passed before the script name, correctly displays the Poetry version and does not execute the script.
tests/console/commands/test_run.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @abn - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟡 Testing: 1 issue found
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

tests/console/commands/test_run.py Show resolved Hide resolved
@abn abn force-pushed the issues/10051 branch 2 times, most recently from aeb98fb to 2a053a7 Compare January 17, 2025 16:31
@abn abn enabled auto-merge (rebase) January 17, 2025 17:22
@abn abn disabled auto-merge January 17, 2025 17:22
@latk
Copy link

latk commented Jan 17, 2025

Thank you for tackling this bug – it's a really thorny issue. I'm still seeing a lot of problems, and provide detailed reproduction steps below. I don't fully understand what's going on, but hope that the test cases help.

I think this means that any command line parameter munging based on heuristics like "there is a token called run" or "we try to parse a substring of tokens" or "we can ignore all tokens after this index" is doomed to fail one way or another. The key problem is that an option like -? or --??? may or may not consume the next token as its value. It is not possible to guess correctly, one must know that specific option. Skipping over options is exactly as hard as actually parsing them, so either Cleo implements the necessary feature to stop option processing when the first argument is found, or Poetry has to faithfully reproduce the Cleo parsing logic to insert the -- in the right place.

Reproduction steps

I tried to exhaustively check different styles of passing the --project parameter, so I installed the development version of Poetry as poetry@9d9cf303, saved the following script as reproducer.sh, and invoked it like bash reproducer.sh poetry@9d9cf303:

#!/bin/bash
set -euo pipefail

poetry="${1:-poetry}"
workdir="$(mktemp -d)"
cd "$workdir"

"$poetry" new run  # create a new package called "run"
ln -sT run somepackage
eza -aT  # show directory structure

experiment() {
  if "$@"; then
    echo PASS: "$*"
  else
    echo FAIL: "$*"
    # exit 1  # uncomment this line to turn into automated test
  fi
}

# start experiments, ALL of these should just echo "hi"
for package in run somepackage; do
  experiment env --chdir=$package "$poetry" run echo hi   # test without -P/--project option

  # option before "run" command
  experiment "$poetry" -P $package run echo hi
  experiment "$poetry" -P$package run echo hi
  experiment "$poetry" --package $package run echo hi
  experiment "$poetry" --package=$package run echo hi

  # option after "run" command
  experiment "$poetry" run -P $package echo hi
  experiment "$poetry" run -P$package echo hi
  experiment "$poetry" run --package $package echo hi
  experiment "$poetry" run --package=$package echo hi
done

cd -
rm -r "$workdir"

The eza command is useful for illustrating the resulting directory layout, but not essential. I've added a somepackage → run symlink in order to easily add examples that don't run into issues with the token run.

Output of my tests:
Created package run in run
.
├── run
│  ├── pyproject.toml
│  ├── README.md
│  ├── run
│  │  └── __init__.py
│  └── tests
│     └── __init__.py
└── somepackage -> run
Creating virtualenv run-OAWMfzkB-py3.12 in /home/user/.cache/pypoetry/virtualenvs
hi
PASS: env --chdir=run poetry@9d9cf303 run echo hi
hi
PASS: poetry@9d9cf303 -P run run echo hi

Specified path '/tmp/tmp.Mq1hAQGOLf/r' is not a valid directory.
FAIL: poetry@9d9cf303 -Prun run echo hi

Poetry could not find a pyproject.toml file in /tmp/tmp.Mq1hAQGOLf or its parents
FAIL: poetry@9d9cf303 --package run run echo hi

Poetry could not find a pyproject.toml file in /tmp/tmp.Mq1hAQGOLf or its parents
FAIL: poetry@9d9cf303 --package=run run echo hi
hi
PASS: poetry@9d9cf303 run -P run echo hi

Specified path '/tmp/tmp.Mq1hAQGOLf/r' is not a valid directory.
FAIL: poetry@9d9cf303 run -Prun echo hi

Poetry could not find a pyproject.toml file in /tmp/tmp.Mq1hAQGOLf or its parents
FAIL: poetry@9d9cf303 run --package run echo hi

Poetry could not find a pyproject.toml file in /tmp/tmp.Mq1hAQGOLf or its parents
FAIL: poetry@9d9cf303 run --package=run echo hi
hi
PASS: env --chdir=somepackage poetry@9d9cf303 run echo hi
hi
PASS: poetry@9d9cf303 -P somepackage run echo hi

Specified path '/tmp/tmp.Mq1hAQGOLf/s' is not a valid directory.
FAIL: poetry@9d9cf303 -Psomepackage run echo hi

The command "somepackage" does not exist.
FAIL: poetry@9d9cf303 --package somepackage run echo hi

Poetry could not find a pyproject.toml file in /tmp/tmp.Mq1hAQGOLf or its parents
FAIL: poetry@9d9cf303 --package=somepackage run echo hi

The command "somepackage" does not exist.
FAIL: poetry@9d9cf303 run -P somepackage echo hi

Specified path '/tmp/tmp.Mq1hAQGOLf/s' is not a valid directory.
FAIL: poetry@9d9cf303 run -Psomepackage echo hi

Poetry could not find a pyproject.toml file in /tmp/tmp.Mq1hAQGOLf or its parents
FAIL: poetry@9d9cf303 run --package somepackage echo hi

Poetry could not find a pyproject.toml file in /tmp/tmp.Mq1hAQGOLf or its parents
FAIL: poetry@9d9cf303 run --package=somepackage echo hi
/tmp

As we can see, most of the invocation styles still fail. It is easier to enumerate what did work:

  • env --chdir=run poetry@9d9cf303 run echo hi
  • poetry@9d9cf303 -P run run echo hi
  • poetry@9d9cf303 run -P run echo hi
  • env --chdir=somepackage poetry@9d9cf303 run echo hi
  • poetry@9d9cf303 -P somepackage run echo hi

For the failures, some seem to truncate the package path to the first letter (before turning it into an absolute path):

Specified path '/tmp/tmp.AZHrkoKFx5/r' is not a valid directory.
  • poetry@9d9cf303 -Prun run echo hi
  • poetry@9d9cf303 run -Prun echo hi
  • poetry@9d9cf303 -Psomepackage run echo hi
  • poetry@9d9cf303 run -Psomepackage echo hi

Other failures seem to ignore the --package option an stay in the $workdir, which is not a Python package:

Poetry could not find a pyproject.toml file in /tmp/tmp.AZHrkoKFx5 or its parents
  • poetry@9d9cf303 --package run run echo hi
  • poetry@9d9cf303 --package=run run echo hi
  • poetry@9d9cf303 run --package run echo hi
  • poetry@9d9cf303 run --package=run echo hi
  • poetry@9d9cf303 --package=somepackage run echo hi
  • poetry@9d9cf303 run --package somepackage echo hi
  • poetry@9d9cf303 run --package=somepackage echo hi

On a few cases, the subcommand detection seems to fail:

The command "somepackage" does not exist.
  • poetry@9d9cf303 --package somepackage run echo hi
  • poetry@9d9cf303 run -P somepackage echo hi

I also repeated the test with Poetry 2.0.1, which passes 6 test cases, which is one more than in 9d9cf30. Specifically, the example poetry run -P somepackage echo hi was lost by the changes in this PR. For the examples where both versions fail, the "command not found" errors are formatted slightly differently, and examples have moved between error categories, but I'm not sure if this is material. The truncated package path error was introduced by this PR and is not present in Poetry 2.0.1.

@abn
Copy link
Member Author

abn commented Jan 18, 2025

I'll look into it. But quite honestly I'll be happy if things are better than it was, and we should be careful of optimizing for edge cases too much. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Related to the command line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

documented CLI option syntax with values doesn't work for subcommands
2 participants