Skip to content

fix: correct the execution of commands and re-organize code#177

Draft
KrLite wants to merge 9 commits intojoshrotenberg:mainfrom
KrLite:fix/command-execution
Draft

fix: correct the execution of commands and re-organize code#177
KrLite wants to merge 9 commits intojoshrotenberg:mainfrom
KrLite:fix/command-execution

Conversation

@KrLite
Copy link
Copy Markdown
Contributor

@KrLite KrLite commented Nov 20, 2025

This replaces all the self.executor.execute_command("docker", args) with self.execute_command(args). The former ones are absolutely incorrect because they ignore the wrapping layer of self.execute_command and generate double docker commands like docker docker pull.

This reveals another problem: how can we integrate this behavior into tests? Currently the command executor is not mocked in tests, so we cannot make a final check of how the commands are actually performed. Maybe we should implement a way of mocking the executor.

@KrLite
Copy link
Copy Markdown
Contributor Author

KrLite commented Nov 20, 2025

This will fix #176

@KrLite
Copy link
Copy Markdown
Contributor Author

KrLite commented Nov 21, 2025

I just found that there are much more problems to solve. WIP-ing this pull request.

@KrLite KrLite marked this pull request as draft November 21, 2025 04:54
@KrLite KrLite changed the title fix: correct the execution of commands fix: correct the execution of commands and re-organize code Nov 21, 2025
@joshrotenberg
Copy link
Copy Markdown
Owner

Thanks for the help!

Would you be willing to split this into separate PRs?

  1. Bug fix PR - Just the first commit fixing self.executor.execute_command("docker", args) to self.execute_command(args). This is ready to merge.

  2. Refactoring PR(s) - The API changes and reorganization can be discussed separately once the bug fix is in.

Happy to merge the bug fix quickly if you can isolate it.

@joshrotenberg
Copy link
Copy Markdown
Owner

I actually went ahead and merged a fix in #180 for the docker docker bug with you as the author so you get proper credit.

For the rest of this PR, I'd love to get some of these improvements in, but would prefer smaller, focused PRs if possible. For example:

  • The get_executor() -> executor() rename is a good Rust idiom fix - that could be its own PR
  • The compose command reorganization into a subdirectory could be another
  • Doc comment standardization could be another

Breaking it up makes it easier to review and merge incrementally, and reduces the risk of conflicts or CI issues. Let me know if you're interested in splitting this up, or if you'd prefer I cherry-pick specific changes.

@KrLite
Copy link
Copy Markdown
Contributor Author

KrLite commented Dec 31, 2025

Thanks for merging the fix! For the rest of the PR, yes I prefer splitting into smaller ones because it's quite a hard work to toggle all the stuffs.

Actually, I'm thinking of creating a generic DSL that accepts command declarations and generates structured, type-safe interfaces. In that case we won't ever worry about architecture, namings and documents and can quickly create command wrappers for any CLI application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants