Make flag support type safe through the sbt plugin

[jedesah]

This has the added advantage of bypassing the huge parsing step that didn’t ever really need to be called, so should make things altogether faster on top of avoiding weird behavior. Does it really make much sense to supply the “-help” in your sbt build?

[nshkrob]

Sounds good in theory; what's the weird behavior you are seeing?

[jedesah]

@nshkrob This was a long time ago, but I believe the "weird" behavior I was refering to is the possibility to pass the "-help" flag or another fairly nonsensical flag to pass to this SettingKey and Scrooge will actually parse and honor the flag.

[nshkrob]

Actually I think it's better to keep flags a String as a way of passing in any possible flag, e.g. --verbose, --default-java-namespace, etc. Removing it would break the people who are using it for these options.

[jedesah]

I disagree. If I remember correctly, most of the command line options don't
make sense for an automated build definition. If verbose and this other one
you mentioned make sense than they should be exposed through an sbt key if
they aren't already.

I am on my phone. I will go through the list when I get back in the office
to make sure my argument is valid. But even if they all make sense. Why
incur the overhead of parsing not too mention the loss of type safety.
On Nov 3, 2014 10:42 AM, "Nik Shkrob" [email protected] wrote:

Actually I think it's better to keep flags a String as a way of passing in
any possible flag, e.g. --verbose, --default-java-namespace, etc. Removing
it would break the people who are using it for these options.

—
Reply to this email directly or view it on GitHub
#149 (comment).

[jedesah]

I would argue that dry-run and version do not make a whole lot of sense for an automated build definition not too mention that most of the flags overlap with existing sbt keys exposed by the scrooge plugin and could lead to confusing behavior if defined twice.

[nshkrob]

The full list of options is at twitter.github.io/scrooge/CommandLine.html. I agree on the dry-run and version options, but there needs to be a way to set the rest of them.

If you provide an alternative way to set all these options this might work. You'll also need to update the docs at https://github.com/twitter/scrooge/blob/master/doc/src/sphinx/SBTPlugin.rst

The performance win after removing option parsing is likely very minor, so I think it isn't worth the loss of flexibility.

[jedesah]

@nshkrob Sorry for the long delay! Got distracted by other things.

[mosesn]

LGTM, let's see if we can merge this in!

[mosesn]

This is extremely stupid, but adding some of these flags tips us over the edge to too many arguments for sbt. It only goes up to 11. So I removed experimental flags because we don't seem to use them anywhere.

[jedesah]

@mosesn Do you want me to rebase this or are you going to do that as part of your internal merging process (twitter open source still works that way?)?

[mosesn]

Yeah, I'll do it as part of the internal merging process, thanks! It's up for review internally now.

[mosesn]

This made it to develop here: 5979724 thanks so much! Really sorry about how long this took.