fix: skip plugin loading for --version/-V flag#10781
Open
majiayu000 wants to merge 2 commits intopython-poetry:mainfrom
Open
fix: skip plugin loading for --version/-V flag#10781majiayu000 wants to merge 2 commits intopython-poetry:mainfrom
--version/-V flag#10781majiayu000 wants to merge 2 commits intopython-poetry:mainfrom
Conversation
Skip loading plugins when --version or -V is passed since plugins cannot affect version output. This reduces startup time by avoiding unnecessary plugin initialization. Signed-off-by: majiayu000 <1835304752@qq.com>
Reviewer's GuideSkips plugin loading when the Poetry application is invoked with top-level version flags ( Sequence diagram for version flag execution and plugin loadingsequenceDiagram
actor User
participant Shell
participant PoetryApplication
participant IO
participant Input
participant PluginManager
User->>Shell: poetry --version
Shell->>PoetryApplication: invoke main
PoetryApplication->>IO: create IO
IO->>Input: parse argv
PoetryApplication->>PoetryApplication: _load_plugins(io)
PoetryApplication->>Input: has_parameter_option("--no-plugins")
Input-->>PoetryApplication: false
PoetryApplication->>Input: has_parameter_option(["--version","-V"], only_params=True)
Input-->>PoetryApplication: true
PoetryApplication-->>PluginManager: skip instantiation
PoetryApplication->>PoetryApplication: mark _plugins_loaded = True
PoetryApplication->>PoetryApplication: resolve version
PoetryApplication-->>IO: write version to output
IO-->>User: display version
PoetryApplication-->>Shell: exit code 0
Class diagram for updated plugin loading logic in applicationclassDiagram
class PoetryApplication {
bool _disable_plugins
bool _plugins_loaded
_load_plugins(io: IO) void
}
class IO {
Input input
}
class Input {
has_parameter_option(values, only_params) bool
}
class PluginManager {
PluginManager() void
load_plugins() void
}
class ApplicationPlugin {
name str
activate() void
}
PoetryApplication --> IO : uses
IO --> Input : wraps
PoetryApplication ..> PluginManager : conditionally_instantiates
PluginManager ..> ApplicationPlugin : manages
note for PoetryApplication "_load_plugins now skips PluginManager instantiation when --no-plugins is set or when --version/-V are present as top-level options (only_params = True)."
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new test reaches into the private
_plugins_loadedattribute to assert behavior; consider asserting the externally observable behavior (e.g., no plugin side effects) instead of relying on an internal implementation detail that may change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new test reaches into the private `_plugins_loaded` attribute to assert behavior; consider asserting the externally observable behavior (e.g., no plugin side effects) instead of relying on an internal implementation detail that may change.
## Individual Comments
### Comment 1
<location path="tests/console/test_application.py" line_range="86-95" />
<code_context>
assert tester.status_code == 0
+@pytest.mark.parametrize("flag", ["--version", "-V"])
+def test_application_with_plugins_skipped_for_version(
+ flag: str, with_add_command_plugin: None, mocker: MockerFixture
+) -> None:
+ app = Application()
+
+ manager_mock = mocker.patch(
+ "poetry.plugins.plugin_manager.PluginManager", autospec=True
+ )
+
+ tester = ApplicationTester(app)
+ tester.execute(flag)
+
+ assert tester.status_code == 0
+ assert app._plugins_loaded is True
+ manager_mock.assert_not_called()
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion that the version output is correct, not just that the exit code is 0
Since this test is meant to validate the version behavior, it should also assert the actual output, not just success and plugin behavior. Please add a check that `tester.io.fetch_output()` (or similar) includes the expected version string (e.g. `__version__` or `app.get_version()`), so changes to the version output are caught by this test.
Suggested implementation:
```python
@pytest.mark.parametrize("flag", ["--version", "-V"])
def test_application_with_plugins_skipped_for_version(
flag: str, with_add_command_plugin: None, mocker: MockerFixture
) -> None:
app = Application()
manager_mock = mocker.patch(
"poetry.plugins.plugin_manager.PluginManager", autospec=True
)
tester = ApplicationTester(app)
tester.execute(flag)
output = tester.io.fetch_output()
assert tester.status_code == 0
assert app._plugins_loaded is True
manager_mock.assert_not_called()
assert __version__ in output
```
To make this compile, ensure this test module imports `__version__` from Poetry's version module near the top of the file, for example:
`from poetry.__version__ import __version__`
If a different version symbol or helper (e.g. `Application().version` or `Application().get_version()`) is already used elsewhere in the tests, adjust the assertion to check for that value instead of `__version__`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+86
to
+95
| @pytest.mark.parametrize("flag", ["--version", "-V"]) | ||
| def test_application_with_plugins_skipped_for_version( | ||
| flag: str, with_add_command_plugin: None, mocker: MockerFixture | ||
| ) -> None: | ||
| app = Application() | ||
|
|
||
| manager_mock = mocker.patch( | ||
| "poetry.plugins.plugin_manager.PluginManager", autospec=True | ||
| ) | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Add an assertion that the version output is correct, not just that the exit code is 0
Since this test is meant to validate the version behavior, it should also assert the actual output, not just success and plugin behavior. Please add a check that tester.io.fetch_output() (or similar) includes the expected version string (e.g. __version__ or app.get_version()), so changes to the version output are caught by this test.
Suggested implementation:
@pytest.mark.parametrize("flag", ["--version", "-V"])
def test_application_with_plugins_skipped_for_version(
flag: str, with_add_command_plugin: None, mocker: MockerFixture
) -> None:
app = Application()
manager_mock = mocker.patch(
"poetry.plugins.plugin_manager.PluginManager", autospec=True
)
tester = ApplicationTester(app)
tester.execute(flag)
output = tester.io.fetch_output()
assert tester.status_code == 0
assert app._plugins_loaded is True
manager_mock.assert_not_called()
assert __version__ in outputTo make this compile, ensure this test module imports __version__ from Poetry's version module near the top of the file, for example:
from poetry.__version__ import __version__
If a different version symbol or helper (e.g. Application().version or Application().get_version()) is already used elsewhere in the tests, adjust the assertion to check for that value instead of __version__.
|
Related Documentation 2 document(s) may need updating based on files changed in this PR: Python Poetry cli
|
Replace _plugins_loaded assertion with version output check to avoid relying on internal implementation details. Signed-off-by: majiayu000 <1835304752@qq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Check List
Resolves: #10625
Summary
poetry --versionis slower than necessary because it loads all plugins even though plugins cannot affect version output.This adds a check for
--version/-Vin_load_plugins()alongside the existing--no-pluginscheck. When either flag is present, plugin loading is skipped entirely.The
only_params=Trueargument matches cleo's own--versiondetection inBaseApplication._run(), sopoetry run -- --versioncorrectly still loads plugins (because--versionappears after the--separator).Single-line condition change, no new dependencies.
Test Plan
Added parametrized test covering both
--versionand-Vflags, verifying:PluginManageris never instantiated_plugins_loadedflag is still set (prevents re-entry)Summary by Sourcery
Skip loading plugins when the application is invoked with the --version/-V flags to speed up version output while preserving existing behavior for other commands.
Bug Fixes:
Tests: