Skip to content
This repository has been archived by the owner on May 24, 2019. It is now read-only.

[WIP] add -r & -l to subcommand create (#12) #15

Conversation

root360-AndreasUlm
Copy link
Contributor

Signed-off-by: Andreas Ulm andreas.ulm@root360.de

Signed-off-by: Andreas Ulm <andreas.ulm@root360.de>
@lunny
Copy link
Member

lunny commented Apr 16, 2019

The parent command CmdReleases has had these two flags?

@root360-AndreasUlm
Copy link
Contributor Author

Yes it does already.

@root360-AndreasUlm
Copy link
Contributor Author

Also when setting this flag before 'create'-subcommand it is recognized and works.

@root360-AndreasUlm
Copy link
Contributor Author

I'm currently testing an approach to simplify using global flags.
I'll create a different PR with the implementation then we can discuss it.

@noerw
Copy link
Member

noerw commented Apr 25, 2019

I was confronted with this as well when implementing #6, and opted to move the extraction logic of global flags to the initCommand() function to avoid repetition, while allowing prefix or postfix flags for 1st level subcommands.
(This however requires that each command adds the respective flag itself, and works not well for 2nd level or deeper subcommands)

As discussed here, subcommands inheriting global flags would be the solution, but was never implemented.
Generally I find https://github.com/spf13/cobra implements this correctly (and in general has better defaults) while providing a very similar interface to urfave/cli, so switching to that might be the simplest option, if we want this fixed properly.

@lafriks
Copy link
Member

lafriks commented Apr 25, 2019

I would not mind against changing to cobra as that is what I usually use myself :)

techknowlogick and others added 6 commits April 25, 2019 13:55
* Combine all coverage file in one command
* remove generate-coverage
* remove unused method: errCheck

Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
Signed-off-by: Andreas Ulm <andreas.ulm@root360.de>
…360-AndreasUlm/tea into release_add_repo_argument
@root360-AndreasUlm
Copy link
Contributor Author

I started implementing Cobra. ( https://github.com/root360-AndreasUlm/tea/tree/12_switch_to_cobra )
There are some changes in the central flag handling required but the interface of Cobra is nice to use and the flag inheritence works in my current tests.
I'll switch all modules to Cobra and create a PR by the end of the week.

@techknowlogick
Copy link
Member

I prefer we keep same cli lib as we can use flags from gitea itself in this lib and share code. This reduce duplication.

@lunny
Copy link
Member

lunny commented Apr 29, 2019

It seems rebase is not correct.

Signed-off-by: Andreas Ulm <andreas.ulm@root360.de>
…360-AndreasUlm/tea into release_add_repo_argument
@root360-AndreasUlm
Copy link
Contributor Author

I prefer we keep same cli lib as we can use flags from gitea itself in this lib and share code. This reduce duplication.

The problem with the current cli lib is that we have to copy every global flag into every subcommand as the inheritence does not work correctly.
The only workaround to reduce redundant code I found so far is to create a variable containing a []cli.Flag{} with all global flags and append that to the flags of the subcommand (based on urfave/cli#585).

@lunny I rebased the branch again on latest master. But I see the changes in README this PR want to do. Shall I create a new PR with cleaned changes?

@techknowlogick
Copy link
Member

We had a similar issue with go-gitea/gitea, however you can see at this commit: go-gitea/gitea@8d0d7bc#diff-7ddfb3e035b42cd70649cc33393fe32cR75 where we have default flags that append to all subcommands.

@root360-AndreasUlm root360-AndreasUlm changed the title add -r & -l to subcommand create (#12) [WIP] add -r & -l to subcommand create (#12) May 2, 2019
@root360-AndreasUlm
Copy link
Contributor Author

is closed in favor of #26 implementation

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants