-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
mark dry run in Poetry output #9973
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request marks dry runs in Poetry output, addressing issue #9972. It modifies several test files to assert the presence of "Running in DRY RUN mode" in the output when the Sequence diagram for dry run mode output in Poetry commandssequenceDiagram
actor User
participant CLI
participant Command
participant IO
User->>CLI: Execute command with --dry-run
CLI->>Command: handle()
alt dry-run option enabled
Command->>IO: write_line('Running in DRY RUN mode')
end
Command->>IO: write regular command output
IO-->>User: Display output
Class diagram showing modified Poetry commandsclassDiagram
class IO {
+write_line(text: str)
}
class Command {
+handle(): int
+option(name: str): Any
#_io: IO
}
class VersionCommand {
+handle(): int
}
class Publisher {
+publish()
#_io: IO
}
class Installer {
+run(): int
+is_dry_run(): bool
#_io: IO
}
Command <|-- VersionCommand
Command --> IO
Publisher --> IO
Installer --> IO
note for Command "Modified to show dry-run mode"
note for Publisher "Added dry-run output"
note for Installer "Added dry-run output"
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 @piotr-kubiak - 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: all looks good
- 🟢 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.
If I recall correctly, the output is intentionally similar. I don't think there is a need for this change. Happy to hear from others before deciding. If we do proceed, I do think the message will have to change. |
I do not have a strong opinion. I see benefits on both sides. |
I don't mind adding some markers about the simulation mode, but I agree the message should be different. |
I am more than happy to change the message, but not sure what it should be.
|
Pull Request Check List
A proposed fix for #9972.
Summary by Sourcery
Tests: