-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(cobra): add bin command #906
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
Conversation
@xenfo is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/vercel/turbo-site/DdUjj21pFd1QEXjR1M35iF6pejGq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general note, it looks like there's a lot going on here. Before we dive into specific code changes, can you describe some of the changes like:
- why we want the
logger
package here, vs the one we're currently using - the intended use for some of the
cmdutil
functions, likecreateTraceFile
,createHeapFile
, etc. as they don't seem to be called in this PR - what's the motivation for the changes to
printf.go
?
It looks like there's also some churn in naming and comments. I'm not necessarily opposed to renaming things, but this might not be the PR to do it. For instance, TurboVersion
-> Version
or Config.IsLoggedIn
-> Config.IsAuthenticated
.
I would expect this PR to be primarily focused around the changes to bin.go
, and have the existing BinCommand
delegate to the cobra-based BinCmd
. Once we iterate our way through the various commands making this change, we can do one or more PRs to tie it all together in main.go
.
This PR is for setting up for more Cobra commands and in the final PR I would link everything up into the root Cobra command. |
I understand that these are setting up for later integration, but if we merge this PR, they are unused code. I would prefer that these utilities get pulled in when they are used, even if just by a single command, rather than in advance. Similarly with the actual |
My thinking was to use this PR to setup for others, after this one I can create all the PRs for the rest of the commands at the same time and everything I need for them is already there. This would be done as soon as this is merged, everything is ready. I also don't see why we would want to mix in Cobra commands with current one as it would require moving code out of the command and making a deps struct of sorts that would be deleted on final linking. If you want me to do this I will though. |
I would look at login as a model here. My goal with requesting it be done this way is to keep |
I think it's best to keep all utility files in this PR so that we can merge other PRs faster. Eg: |
It is unfortunately not really possible to review unused code being added, since part of what the review includes is "do we need this code". For utilities that you would like to merge first, especially if they would be useful for There's a lot of surface area to this feature. It encompasses nearly the entire UX of the CLI. So I apologize if merging ends up going a little slower, but I think the priority has to be stability vs the overall speed of merging the PRs. |
Alright, I've reduced this PR to only the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sticking with this. I took a first pass at reviewing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change currently affects 29 files. I think for the task of moving bin
to cobra
, we ideally want to just modify bin.go
. would it be possible to remove the unrelated changes so we can focus on the cobra
usage?
I'll create another PR for formatting then. Edit: #927 |
If we're going to be moving to For example with the new UI we can simply do this to pass an error up. It's one line, it's always in sync and has minimal code duplication. // ch.Ui is the new UI in this example
// for exit code 1
return ch.Ui.Errorf("${UNDERLINE}something went wrong${RESET}")
// OR
// for any other exit code
return &cmdutil.Error{
ExitCode: 123,
Err: ch.Ui.Errorf("${UNDERLINE}something went wrong${RESET}"),
} It's counter part would be this, which has a lot of fluff and has more chances for getting out of sync or some other errors happen. // ch.Ui is the old UI in this example
msg := util.Sprintf("${UNDERLINE}something went wrong${RESET}")
ch.Ui.Error(msg)
return fmt.Errorf(msg)
// OR
msg := util.Sprintf("${UNDERLINE}something went wrong${RESET}")
ch.Ui.Error(msg)
return &cmdutil.Error{
ExitCode: 123,
Err: msg,
} Furthermore, all the commands are already written with new new UI, and changing back to the old one would require me to rewrite the commands again with the old UI. Then if we do want to use the new UI, we would have to rewrite the commands once more. That would be 3 total rewrites vs 1 total rewrite. I understand your trying to keep this PR simple and just the bare minimum for the |
I understand it may be frustrating, but I cannot approve starting a All of the migrations, including this one, are being done via scoped, small PRs. This has a few advantages: easier to review and spot bugs, and in the event that a bug is not caught, a smaller blast radius in terms of what needs to be reverted. I would be happy to review PRs switching individual commands over to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming along!
Co-authored-by: Greg Soltis <gsoltis@gmail.com>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, it looks like there's a minor merge conflict in main.go
, then let's get this merged!
@xenfo can you also run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get this merged! Thanks for sticking with it!
What this PR brings:
bin
command