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 code #56

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

refactor CLI code #56

wants to merge 2 commits into from

Conversation

nickswift
Copy link

Refactor pkl-gen-go.go to be more in line with idiomatic Go recommendations, and to lean more on the functionality provided by Cobra to reduce the complexity of this code.

This includes:

  • Avoiding global state where possible
  • Setting up resources in PreRunE and passing essential values to RunE via the command context.
  • Delegating more of the argument handling and flag-binding logic to Cobra where possible.

This also touches on a similar area as #53, but the changes here are more comprehensive. I do think both changes can co-exist happily with a couple minor changes to this PR, but there should probably be some discussion first.

flagSet := cmd.Flags()

// resolve flag values
printVersion := unwrapValue(flagSet.GetBool(flagNamePrintVersion))
Copy link
Author

@nickswift nickswift Apr 25, 2024

Choose a reason for hiding this comment

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

This bit is where something more like what exists in #53 would be useful. Instead of eating the error with unwrapValue, we could have a constructor that creates a structure with all the CLI flag values in it that returns an error that we can handle properly.

This structure could also hold generator settings and the output path so that all we need to pass through the context is a reference to a single structure (as opposed to three separate values).

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.

1 participant