Feature:(issue_269) Allow external package flag definitions #1540
Conversation
dearchap
changed the title
app.go
Outdated
| if !strings.HasPrefix(f.Name, "test.") { | ||
| a.Flags = append(a.Flags, extFlag{f}) |
There was a problem hiding this comment.
I'm good with specifically ignoring flags with "test." prefix in this implementation 👍🏼 I looked briefly and couldn't find any exported code that could be used to get this prefix from go libraries 🤷🏼
What's the reasoning behind using a extFlag{f} instead of &extFlag{f} and having the extFlag methods use a pointer receiver?
There was a problem hiding this comment.
We need the test prefix check because otherwise when we run unit tests those flags get added !!!!
Also extFlag seemed better since we are not necessarily modifying any aspect of the underlying flag
There was a problem hiding this comment.
@dearchap I'm in favor of filtering out `"test." in general 👍🏼 My thinking was more around if we could read this value from somewhere instead of having it hard-coded here 🤷🏼
As for extFlag, I meant that I wondered if you meant to use the struct and method receivers as structs rather than pointers to structs. In this implementation, there's going to be a lot more implicit copying of structs, IIUC.
There was a problem hiding this comment.
@meatballhat do you want it to be defined as an App field or as a library var ?
There was a problem hiding this comment.
@meatballhat It seems that the go is also the hard-coded way to use directly
https://github.com/golang/go/blob/7e72d384d66f48a78289edc6a7d1dc6ab878f990/src/cmd/go/internal/test/test.go#L1351-L1367
There was a problem hiding this comment.
@meatballhat do you want it to be defined as an App field or as a library var ?
@dearchap Maybe he means use pointer receivers(*extFlag) rather than value receivers(extFlag) implement Flag interface to avoid unnecessary data duplication.
There was a problem hiding this comment.
No no that is fine. I meant the "test" prefix.
There was a problem hiding this comment.
@meatballhat I updated the extFlag to be pointer receiver.
What type of PR is this?
(REQUIRED)
What this PR does / why we need it:
(REQUIRED)
Which issue(s) this PR fixes:
(REQUIRED)
Fixes #269
Special notes for your reviewer:
(fill-in or delete this section)
Testing
go test -run=TestApp_FlagsFromExtPackage
(fill-in or delete this section)
Release Notes
(REQUIRED)