Skip to content

Conversation

@mdrohmann
Copy link
Contributor

@mdrohmann mdrohmann commented Feb 3, 2021

@mdrohmann mdrohmann force-pushed the martind/no-verbose-on-shim-args-176667499 branch from 797b834 to b760e0b Compare February 3, 2021 23:50
@mdrohmann mdrohmann requested a review from MDrakos February 3, 2021 23:51
Copy link
Member

@MDrakos MDrakos left a comment

Choose a reason for hiding this comment

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

I believe cobra should be handling this for us here https://github.com/ActiveState/cli/blob/master/cmd/state/internal/cmdtree/shim.go#L43 but perhaps it's not being called any longer? I noticed that shim has a --path flag so disabling flag parsing may interfere with that. I'm not sure that shim logic should live in main. Can you take a look into this and see if disableFlagParsing is working as intended?

@mdrohmann
Copy link
Contributor Author

mdrohmann commented Feb 4, 2021

The issue here, is that the check for verbose arguments is done even before the cmdtree is initialized. So, this parsing is completely separate from cobra:

Here is where the cmdtree is initialized:
https://github.com/ActiveState/cli/blob/master/cmd%2Fstate%2Fmain.go#L167
And the verbose check is on line 102 above that.

@mdrohmann mdrohmann requested a review from MDrakos February 4, 2021 18:09
@mdrohmann
Copy link
Contributor Author

@Naatan The Windows shim also uses the double dash separator: https://github.com/ActiveState/cli/blob/master/assets%2Fshim%2Fshim.bat#L5

Copy link
Member

@MDrakos MDrakos left a comment

Choose a reason for hiding this comment

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

The issue here, is that the check for verbose arguments is done even before the cmdtree is initialized. So, this parsing is completely separate from cobra

I see what you mean. I will defer to @Naatan on this one. I still feel that we should be handling the flags all in one place, but that should probably be it's own ticket.

@mdrohmann mdrohmann merged commit 58b95bd into master Feb 4, 2021
@mdrohmann mdrohmann deleted the martind/no-verbose-on-shim-args-176667499 branch February 4, 2021 20:02
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