-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add poetry wrapper command to generate version-pinned script #10335
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
Conversation
Reviewer's Guide by SourceryThis pull request introduces a Sequence diagram for generating the poetry wrappersequenceDiagram
participant User
participant PoetryCLI
participant WrapperCommand
participant FileSystem
User->>PoetryCLI: poetry wrapper --poetry-version=2.1.2
PoetryCLI->>WrapperCommand: handle()
WrapperCommand->>WrapperCommand: _is_valid_version("2.1.2")
alt Version is valid
WrapperCommand->>FileSystem: Check if poetryw and poetry-wrapper.properties exist
alt Files exist
WrapperCommand->>PoetryCLI: Output warning about overwriting files
end
WrapperCommand->>FileSystem: Write poetry-wrapper.properties with version=2.1.2
WrapperCommand->>WrapperCommand: _generate_script_content("2.1.2")
WrapperCommand->>FileSystem: Write poetryw script
WrapperCommand->>WrapperCommand: _make_executable(poetryw)
WrapperCommand->>PoetryCLI: Output success message and file locations
else Version is invalid
WrapperCommand->>PoetryCLI: Output error message
end
Sequence diagram for executing the poetryw scriptsequenceDiagram
participant User
participant poetryw
participant FileSystem
participant Poetry
User->>poetryw: ./poetryw install
poetryw->>FileSystem: Check if poetry-wrapper.properties exists
alt File exists
poetryw->>FileSystem: Read version from poetry-wrapper.properties
poetryw->>poetryw: Set POETRY_HOME to version-specific cache
poetryw->>FileSystem: Check if Poetry is installed in POETRY_HOME
alt Poetry is not installed
poetryw->>poetryw: Install Poetry version from install.python-poetry.org
end
poetryw->>Poetry: Execute Poetry with provided arguments (install)
Poetry-->>User: Output from Poetry
else File does not exist
poetryw->>User: Output error message
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @ranzyblessings - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a check for the existence of the poetry executable after installation in the generated script.
- The documentation update includes some unnecessary whitespace changes; consider reverting those.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Add
|
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.
Hey @ranzyblessings - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a more robust version validation using
packaging.version.parsefrom thepackaginglibrary. - The generated script does not handle spaces in the project path; consider quoting the path to
$POETRY_HOME.
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
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Implement the `poetry wrapper` command to generate a `poetryw` Bash script and `poetry-wrapper.properties` file, enabling version-pinned Poetry usage on Linux and macOS. The generated script ensures consistent Poetry versions across environments without requiring global installation, streamlining development and CI/CD workflows. Features: - Generates a Bash script (`poetryw`) that installs and runs a specified Poetry version. - Supports custom versions via `--poetry-version` (e.g., `2.1.2`). - Validates semantic version format (X.Y.Z). - Overwrites existing files with a warning. - Sets executable permissions on POSIX systems. Limitations: - Only supports Linux and macOS (Windows requires additional work). - Does not verify Poetry installer integrity (e.g., checksums). Tests cover version validation, script generation, file permissions, and error cases (invalid versions, file write failures). Signed-off-by: Ranzy Blessings <[email protected]>
Document the `poetry wrapper` command in `docs/cli.md` to provide clear usage instructions and reference material for pinning Poetry versions. The documentation details the command’s purpose, options, generated files, and platform-specific notes, ensuring developers can effectively leverage the wrapper for consistent project environments. Features: - Describes generating `poetryw` script and `poetry-wrapper.properties` file. - Includes usage syntax, `--poetry-version` option, and practical examples. - Clarifies Unix-only support and overwrite behavior. - Notes version format validation (X.Y.Z) without registry checks. Formatted to align with Poetry’s `cli.md` style, maintaining consistency with existing command documentation for seamless integration. Signed-off-by: Ranzy Blessings <[email protected]>
This reverts commit e148bab.
- Added post-installation check for poetry executable in wrapper script - Simplified chmod permissions to 0o755 for better security - Removed unnecessary whitespace changes in docs/cli.md - Added tests for invalid version edge cases and script write errors - Updated wrapper script tests to verify new installation checks Signed-off-by: Ranzy Blessings <[email protected]>
…ed on Sourcery-AI Suggestions - Refactored `WrapperCommand` class to properly annotate mutable class attributes with `ClassVar` to adhere to type-checking guidelines. - Enhanced wrapper script to validate version format more robustly with basic semantic checks (X.Y.Z format). - Updated tests to replace nested `with` statements with combined contexts for cleaner code and better readability. - Fixed missing newline at end of `cli.md` to align with documentation standards. Signed-off-by: Ranzy Blessings <[email protected]>
- Refactored `test_handle_with_invalid_version` and `test_invalid_version_edge_cases` by combining nested `with` statements into a single `with` block, improving code clarity. - Streamlined the patching process by merging multiple `patch` calls into one, reducing redundancy and enhancing maintainability. - This change enhances code readability and aligns with best practices for cleaner, more efficient test code. *Suggested by Sourcery AI for readability and performance improvements.* Signed-off-by: Ranzy Blessings <[email protected]>
…lity - Combined nested `with` statements into a single `with` block in `test_handle_properties_file_write_error` to address Ruff SIM117 warning. - Improved code clarity and efficiency by streamlining patching process. - Aligned with Ruff formatting standards to ensure consistency. *Suggested by Sourcery AI for readability and performance improvements.* Signed-off-by: Ranzy Blessings <[email protected]>
…able - Refactored `_make_executable` in `WrapperCommand` to check `os.name` before attempting `chmod`, ensuring consistent behavior on non-POSIX systems like Windows by emitting a warning without relying on `OSError`. - Added `test_make_executable_on_non_posix` to verify that `_make_executable` on non-POSIX systems outputs the expected warning and preserves file read/write permissions, enhancing test coverage. - Simplified error handling in `_make_executable` for clarity and reliability, aligning with robust platform-agnostic design principles. - Suggested by Sourcery-AI: Improved code robustness and test reliability for cross-platform support. Signed-off-by: Ranzy Blessings <[email protected]>
|
Hi @python-poetry/triage, just checking in on this PR. I’ve addressed Sourcery AI’s feedback and ensured CI checks pass. Would appreciate any guidance or next steps. Thanks! |
|
This looks more like a plugin candidate, especially since it introduces a lot of new code to maintain (including bash/sh code, which tends to have many quirks). It's more of a puppy, not a cake (see Will's post) |
|
Thanks for the feedback, @Secrus. Understood that this might fit better as a plugin. I’ll consider my options given time constraints. Appreciate the review! |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Implement the
poetry wrappercommand to generate apoetrywBash script andpoetry-wrapper.propertiesfile, enabling version-pinned Poetry usage on Linux and macOS. The generated script ensures consistent Poetry versions across environments without requiring global installation, streamlining development and CI/CD workflows.Features:
poetryw) that installs and runs a specified Poetry version.--poetry-version(e.g.,2.1.2).Limitations:
Tests cover version validation, script generation, file permissions, and error cases (invalid versions, file write failures).
Summary by Sourcery
Implement a new
poetry wrappercommand to generate a version-pinned Poetry execution script for Unix-like systemsNew Features:
poetrywscript to ensure consistent Poetry version usage across environmentsDocumentation:
wrappercommand in CLI documentation, explaining usage, generated files, and system limitationsTests: