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

Command Refactor #388

Merged
merged 2 commits into from
May 8, 2024
Merged

Command Refactor #388

merged 2 commits into from
May 8, 2024

Conversation

phalestrivir
Copy link
Contributor

This PR updates the help command by adding a custom Help object for FrodoStubCommands. Additionally, it refactors all sub-commands to run in the same process instead of running the sub-commands as executables in separate processes. Thus, everything should be functionally the same besides this change to how the commands are run.

Due to the changes to how the commands are created, there can no longer be two descriptions for each sub-command, but just one description. This isn't a huge deal since a lot of the descriptions were the same or very similar anyways, but it did mean that we had to update the client_cli snapshots where the descriptions differed. Similarly, due to the changes in the help command, the client_cli snapshots had to be updated.

No changes needed to be made to e2e tests or snapshots however. They were originally failing after the changes due to the mock naming scheme no longer working as it was before, likely due to the fact that it is now using only one process to run all sub-commands, resulting in the mocks generating differently now. We made a temporary fix in frodo-lib to how mocks are named to make it so the tests work with the current mocks, which fix is in another PR on the frodo-lib project, although the mock naming is something we will want to improve eventually.

@vscheuber
Copy link
Contributor

@phalestrivir would you mind rebasing and updating your PR? The PR looks generally great but I noticed 43 failing tests, all client_cli/en/* tests with minor help text updates I made over the past little while and I am wondering what other changes might have not made it into your fork? Just a quick sanity check would help.

@phalestrivir
Copy link
Contributor Author

@phalestrivir would you mind rebasing and updating your PR? The PR looks generally great but I noticed 43 failing tests, all client_cli/en/* tests with minor help text updates I made over the past little while and I am wondering what other changes might have not made it into your fork? Just a quick sanity check would help.

I tried rebasing but it says the PR is up to date with the Rockcarver main branch (which is what we branched off of), so I'm not sure what changes you are referring to that might not have made it in, unless they are in some other branch besides main. I re-ran all the client_cli/en/* tests on my end just to make sure and they are all passing for me.

@vscheuber
Copy link
Contributor

vscheuber commented May 7, 2024 via email

@phalestrivir
Copy link
Contributor Author

I noticed the Actions are failing for this PR. I looked at when the frodo-lib changes were added and they were added after the actions ran for this PR, so if you run the Actions now they should all pass.

@vscheuber vscheuber merged commit 0553f77 into rockcarver:main May 8, 2024
8 of 11 checks passed
@phalestrivir phalestrivir deleted the command-refactor branch June 25, 2024 17:47
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.

3 participants