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

feat: Nested Commands & Project Group Create #109

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

domenicsim1
Copy link
Collaborator

No description provided.

@domenicsim1 domenicsim1 requested a review from a team October 11, 2022 01:31
CmdPath string
}

func (co *CreateOptions) Commit() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the commit pattern for each command

Copy link
Contributor

@benPearce1 benPearce1 left a comment

Choose a reason for hiding this comment

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

🏆 this looks awesome

@domenicsim1 domenicsim1 merged commit 98d10ca into bp/project-create Oct 11, 2022
@domenicsim1 domenicsim1 deleted the feat/nested-commands branch October 11, 2022 01:49
@@ -0,0 +1,6 @@
package cmd

type NestedOpts interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does the same job as the TaskExecutor abstraction.

The advantage this has is that it's simpler because you're sticking commands directly in the collection, rather than having a separation between gather input / perform action.
The disadvantage this has is that the questions are bound (via a closure) to the commit. You can't ask the question differently in a nested command that you could at the root level.

The reason I added that separation for TaskExecutor was to avoid circular package references, to help provide some code structure as the project grew, and to allow for nested questions to be asked differently, but share the underlying 'send to server' bit.

I don't mind that you've gone a different way here, but it would be good to understand.

  • Why didn't you just use the TaskExecutor pattern?
  • This appears to force nested questions to be asked in the same way as they are at the root. Is this worth it?
  • Have we thought through whether there are any other differences that might be significant?

If the team would like to go with this approach, then it obsoletes the TaskExecutor design. We should pay down our tech-debt and delete all that stuff.

for _, o := range optsArray {
o.GenerateAutomationCmd()
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

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.

4 participants