Remove implicit magic with flag registration in generators

[sttts]

This blocks further work on a Golang variant of kubernetes/kubernetes#52186
because we cannot instantiate multiple generators in one process.

[k8s-reviewable]

This change is 

[sttts]

/assign @thockin

[lavalamp]

The point of the args package was so callers can do this boilerplate in like 1 line. Can you find a way to do this such that callers only add one line, or none (like, a function they can call on the thing that Default() returns)?

[thockin]

ACK the goal, but flag processing really belongs to the app, and the fact that gengo knows pflag is sort of awful.

But this is a breaking change, and it obviously doesn't catch all callers. If we're going to factor this way, we should add a new method that doesn't change flags, or at least rename the Default function so callers don't suddenly stop getting flags registered. Or pass a FlagAdder interface to Default which will similarly break compilation, but let us inject pflag from caller.

[sttts]

Backwards compatibility is a good point. An interface sounds overkill. Will add another func.

[thockin]

If adding the interface means we can get rid of one hard dep on pflag, I think we should do it.

I don't care a ton about API compat here (I know that is evil, but this is a niche lib). Simply adding an arg will force callers to adapt explicitly. I just dont' want to have callers compile but not work any more.

[sttts]

Did a minimal version now with a WithoutDefaultFlagParsing() method, disabling the current behaviour, completely optional, keeping @lavalamp's one-liner semantics by default. The old generators are unchanged.

I also tried with interfaces, but everything I came up with was ugly. Moreover, with an AddFlags method we have to keep flags as dependency somewhere. Moreover, due to StringSliceVar we cannot even easily switch to Golang's flag without breaking backwards code compatiblity. I don't care enough about the pflag dependency to uglify the types for the sake of removing the pflag import.

[lavalamp]

I agree with everything you said.

…

On Thu, Sep 28, 2017 at 10:35 AM, Tim Hockin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In examples/deepcopy-gen/main.go <#80 (comment)>: > @@ -76,6 +78,11 @@ func main() { "Comma-separated list of import paths which bound the types for which deep-copies will be generated.") arguments.CustomArgs = customArgs + // Register default flags. ACK the goal, but flag processing really belongs to the app, and the fact that gengo knows pflag is sort of awful. But this is a breaking change, and it obviously doesn't catch all callers. If we're going to factor this way, we should add a new method that doesn't change flags, or at least rename the Default function so callers don't suddenly stop getting flags registered. Or pass a FlagAdder interface to Default which will similarly break compilation, but let us inject pflag from caller. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#80 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnglsiaDZMm0yU7T3z4ffhahs09CaDJks5sm9jGgaJpZM4Pm8sR> .

[thockin]

/lgtm
/approve