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

Consider removing flag auto-generation? #870

Closed
coilysiren opened this issue Aug 19, 2019 · 7 comments
Closed

Consider removing flag auto-generation? #870

coilysiren opened this issue Aug 19, 2019 · 7 comments
Assignees
Labels
kind/cleanup describes internal cleanup / maintaince status/claimed someone has claimed this issue

Comments

@coilysiren
Copy link
Member

So @asahasrabuddhe dramatically improved the flag generation in this PR #836. I think that PR was a necessary first step in us rethinking the flag generation, so I super appreciate that work and love that it was done ✨

I think what we've been doing lately is expanding upon that idea behind that PR ^ and "normalizing" the flag code. So the next step in this process is: should we consider removing the flag generation entirely? I think we could keep the essence of flag generation, by adding tests that ensure the flags all have some attributes defined.

Some motivations behind removing auto-generation:

  • it many cases you don't end up writing any less code to implement a feature, so auto-generation isn't saving on LOC
  • auto-generation is high complexity, and hard to describe to contributors
  • given that we are in total control of the inputs and outputs, there's no particular forcing factor that requires this package have auto-generation

@AudriusButkevicius has mentioned supporting this in passing.

@asahasrabuddhe
Copy link
Member

I didn't understand why the flags were generated in the first place. I support removing this and would like to contribute to this.

@coilysiren
Copy link
Member Author

🚀🚀🚀 @asahasrabuddhe 🚀🚀🚀

@coilysiren coilysiren added status/claimed someone has claimed this issue kind/cleanup describes internal cleanup / maintaince and removed kind/discussion is for discussion labels Aug 20, 2019
@asahasrabuddhe
Copy link
Member

on it

@asahasrabuddhe
Copy link
Member

asahasrabuddhe commented Sep 7, 2019

I have started to work on this but I am still getting an error in the test cases

=== RUN   TestToMan
--- FAIL: TestToMan (0.00s)
    helpers_test.go:20: (docs_test.go:68) Expected .TH "greet" "8" " " 
        .nh
        .ad l
        .TH "Harrison" 
        .nh
        .ad l
        
        
        .SH NAME
        .PP
        greet \- Some app
        
        
        .SH SYNOPSIS
        .PP
        greet
        
        .PP
        .RS
        
        .nf
        [\-\-another\-flag|\-b]
        [\-\-flag|\-\-fl|\-f]=[value]
        [\-\-socket|\-s]=[value]
        
        .fi
        .RE
        
        
        .SH DESCRIPTION
        .PP
        app [first\_arg] [second\_arg]
        
        .PP
        \fBUsage\fP:
        
        .PP
        .RS
        
        .nf
        greet [GLOBAL OPTIONS] command [COMMAND OPTIONS] [ARGUMENTS...]
        
        .fi
        .RE
        
        
        .SH GLOBAL OPTIONS
        .PP
        \fB\-\-another\-flag, \-b\fP: another usage text
        
        .PP
        \fB\-\-flag, \-\-fl, \-f\fP="":
        
        .PP
        \fB\-\-socket, \-s\fP="": some 'usage' text (default: value)
        
        
        .SH COMMANDS
        .SH config, c
        .PP
        another usage test
        
        .PP
        \fB\-\-another\-flag, \-b\fP: another usage text
        
        .PP
        \fB\-\-flag, \-\-fl, \-f\fP="":
        
        .SS sub\-config, s, ss
        .PP
        another usage test
        
        .PP
        \fB\-\-sub\-command\-flag, \-s\fP: some usage text
        
        .PP
        \fB\-\-sub\-flag, \-\-sub\-fl, \-s\fP="":
        
        .SH info, i, in
        .PP
        retrieve generic information
        
        .SH some\-command (type string) - Got .nh
        .TH greet(8) 
        
        .SH Harrison
        
        .SH NAME
        .PP
        greet \- Some app
        
        
        .SH SYNOPSIS
        .PP
        greet
        
        .PP
        .RS
        
        .nf
        [\-\-another\-flag|\-b]
        [\-\-flag|\-\-fl|\-f]=[value]
        [\-\-socket|\-s]=[value]
        
        .fi
        .RE
        
        
        .SH DESCRIPTION
        .PP
        app [first\_arg] [second\_arg]
        
        .PP
        \fBUsage\fP:
        
        .PP
        .RS
        
        .nf
        greet [GLOBAL OPTIONS] command [COMMAND OPTIONS] [ARGUMENTS...]
        
        .fi
        .RE
        
        
        .SH GLOBAL OPTIONS
        .PP
        \fB\-\-another\-flag, \-b\fP: another usage text
        
        .PP
        \fB\-\-flag, \-\-fl, \-f\fP="":
        
        .PP
        \fB\-\-socket, \-s\fP="": some 'usage' text (default: value)
        
        
        .SH COMMANDS
        .SH config, c
        .PP
        another usage test
        
        .PP
        \fB\-\-another\-flag, \-b\fP: another usage text
        
        .PP
        \fB\-\-flag, \-\-fl, \-f\fP="":
        
        .SS sub\-config, s, ss
        .PP
        another usage test
        
        .PP
        \fB\-\-sub\-command\-flag, \-s\fP: some usage text
        
        .PP
        \fB\-\-sub\-flag, \-\-sub\-fl, \-s\fP="":
        
        .SH info, i, in
        .PP
        retrieve generic information
        
        .SH some\-command (type string)
FAIL

Process finished with exit code 1

@saschagrunert would really appreciate your help here.

UPDATE

This issue is not being reproduced on Travis but still is a problem on my system. Could you help me investigate as to why?

UPDATE 2

This issue is being reproduced with Go 1.13

@saschagrunert
Copy link
Member

Hm, it might be related to the indirect blackfriday dependency of go-md2man. Can you try to remove $GOPATH/pkg/mod folder completely and re-test it again? Does it still occur?

@asahasrabuddhe
Copy link
Member

The issue here was inconsistent use of Go Module system. I dropped in an environment variable GO111MODULE=on which made a lot of difference. Earlier it would run the tests using a different version of dependencies for different Go versions. Now that has been fixed.

I, however, have a new problem related to short flags now

@asahasrabuddhe
Copy link
Member

This is done and released as a part of v1.22.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup describes internal cleanup / maintaince status/claimed someone has claimed this issue
Projects
None yet
Development

No branches or pull requests

3 participants