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

refactor(cli): use command runner to run predictable logic #4176

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Oct 4, 2024

Summary

The CLI that runs the commands has become very repeated. I had attempted few refactors before, and eventually landed on this one.

As for now, this refactor only touches the format command, to show how it has become. We can discuss the architecture here, and one we merge this PR, I will refactor the rest of the commands.

We now have a CommandRunner trait that all the payloads need to implement in order to be run. Eventually, only the run command is called, then the rest is just repeated logic that was moved from format.rs.

Pros

With a trait, now the bulk of the logic lives inside one single function, and it is shared among commands. This should reduce the repeated work, especially when we add new CLI options that affect multiple commands e.g. VCS integration.

Cons

When merging the configuration, because we use &mut self, we need to use clone() every time we need to merge CLI options into the configuration loaded from disk.

Test Plan

The existing tests should pass

@github-actions github-actions bot added the A-CLI Area: CLI label Oct 4, 2024
@ematipico ematipico merged commit 64a84a6 into main Oct 7, 2024
9 of 10 checks passed
@ematipico ematipico deleted the refactor/command-run branch October 7, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants