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

Split up some large command handlers #96

Merged
merged 2 commits into from
Sep 22, 2023

Conversation

itowlson
Copy link
Contributor

This PR does two things (in two separate commits, as all of this is likely to be subjective). It is purely a refactoring and should not change any existing behaviour. (Okay, okay. In the vanishingly implausible cases where an app has an empty URL, the print behaviour changes slightly. Truth in advertising.)

  1. It splits the apps and sqlite command handlers into subcommand handlers. This removes two levels of indentation from the actual command handling code, to my eye making it easier to track, and creates some visual separation between the subcommand blocks.
  2. It streamlines (in my opinion of course!) the rendering part of the spin cloud apps info command by moving some logic into helper functions, and teasing apart the "what do we want to print" from the actual printing. I feel this makes it easier to read as a "top to bottom" function by reducing the nested ifs/matches.

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
Copy link
Collaborator

@karthik2804 karthik2804 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for this.

As a follow-up - Do we want to use the same structure of cmd.run() for the variables subcommand as well?
https://github.com/fermyon/cloud-plugin/blob/main/src/commands/variables.rs

It does look like the functions there are pretty small as they are today though.

@itowlson
Copy link
Contributor Author

Yes, I didn't feel it was worth it for 4-7 lines per block - the logic is mostly farmed out already, just to free functions rather than subcommand associated functions. I'm happy to do it though if folks feel it worthwhile.

@karthik2804
Copy link
Collaborator

That makes sense to me!

@itowlson itowlson merged commit 803c10b into fermyon:main Sep 22, 2023
8 checks passed
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.

None yet

2 participants