Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Consistent option naming #1084

Merged
merged 3 commits into from
Jul 7, 2015
Merged

Consistent option naming #1084

merged 3 commits into from
Jul 7, 2015

Conversation

squaremo
Copy link
Contributor

@squaremo squaremo commented Jul 6, 2015

Namely:

  • long options get two hyphens at the start; e.g., --password; and,
  • options made from multiple whole words are hyphenated; e.g., --no-discovery; and,
  • some options that are applied to DNS but sound universal have dns prepended, e.g., --dns-port.

There are exceptions; "ipalloc" is treated as one word for hyphenation; the --tls* options are left as-is since they correspond to Docker options; some niche options don't get hyphenated e.g., --bufsz; some options that are likely to disappear are left alone.

In all cases, the old version of an option will work, while outputting a deprecation warning. Mostly these will end up in container logs. Renamed options that are mentioned in the script usage, or otherwise specially-treated in the script, have a deprecation notice output to the console.

Closes #602 and closes #1035.

@rade
Copy link
Member

rade commented Jul 6, 2015

So no --option= except for the tls args?

@squaremo
Copy link
Contributor Author

squaremo commented Jul 6, 2015

So no --option= except for the tls args?

The mflag library will in general treat --option=value and --option value as the same. The exception, as usual, is boolean flags, where the --option bool variety is ruled out.

@rade
Copy link
Member

rade commented Jul 6, 2015

The mflag library will in general treat --option=value and --option value as the same

Yes, but the script doesn't.

@squaremo
Copy link
Contributor Author

squaremo commented Jul 6, 2015

Yes, but the script doesn't.

Oh I see. Are you saying it did before and it doesn't now?

@rade
Copy link
Member

rade commented Jul 6, 2015

Are you saying it did before and it doesn't now?

No, it didn't, but it seems odd for the exe to support it (if it did so previously then that was by accident) but not the script. Same goes for any short options.

@squaremo
Copy link
Contributor Author

squaremo commented Jul 6, 2015

No, it didn't, but it seems odd for the exe to support it (if it did so previously then that was by accident) but not the script.

The script doesn't in general support it because no-one thought to implement it (probably because it's a bit of a pain, unless you happen to be messing around with the arguments anyway, as in the case of --tls*).

The exes do support that style because the mflag package happens to support that. As does the flag package. In other words, nothing has changed on that front.

@rade
Copy link
Member

rade commented Jul 6, 2015

The script doesn't in general support it because no-one thought to implement

I actually didn't know it worked.

probably because it's a bit of a pain

Only very slightly. I see no reason why the launch-router arg processing couldn't handle this in very much the same way as proxy_args and dns_args.

@squaremo
Copy link
Contributor Author

squaremo commented Jul 6, 2015

OK, I agree that would be nice, but I'm not trying to address that here, only to rename the options in a backwards-compatible way.

@rade rade mentioned this pull request Jul 6, 2015
Closed
@rade
Copy link
Member

rade commented Jul 6, 2015

@paulbellamy would you mind reviewing this?

@bboreham
Copy link
Contributor

bboreham commented Jul 6, 2015

Is this also supposed to close #1035?

@squaremo
Copy link
Contributor Author

squaremo commented Jul 6, 2015

Is this also supposed to close #1035?

That was the thing I actually wanted to fix, yes good point.

rade
rade reviewed Jul 6, 2015
View reviewed changes
weave
echo "weave launch [--password <password>] [--nickname <nickname>] [--no-discovery]"
echo " [--ipalloc-range <cidr> [--ipalloc-default-subnet <cidr>]]"
echo " [--init-peer-count <count>] <peer> ..."
echo "weave launch-router [--password <password>] [--nickname <nickname>]"

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@squaremo squaremo force-pushed the issue602_moar_hyphens branch from 27e7b43 to af6f421 Compare July 7, 2015 08:30
bboreham
bboreham reviewed Jul 7, 2015
View reviewed changes
prog/weaveproxy/main.go
@@ -15,6 +15,23 @@ var (
defaultListenAddrs = []string{"tcp://0.0.0.0:12375", "unix:///var/run/weave.sock"}
)

type listOpts struct {
values *[]string

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@squaremo squaremo force-pushed the issue602_moar_hyphens branch from af6f421 to c9145e8 Compare July 7, 2015 08:53
rade
rade reviewed Jul 7, 2015
View reviewed changes
weave
shift
done
}

This comment was marked as abuse.

Sign in to view

@paulbellamy
Copy link
Contributor

There are remaining references to single-hyphen flags in:
bin/multiweave:33
docs/dockerless.txt:128-129
proc/weavedns/main.go:58 (in the help text)
prog/weaver/main.go:96,149,158
site/ipam.md:140

@squaremo
Copy link
Contributor Author

squaremo commented Jul 7, 2015

There are remaining references

Nice spotting, thank you :)

@squaremo squaremo force-pushed the issue602_moar_hyphens branch from c9145e8 to 2b2c892 Compare July 7, 2015 10:12
@squaremo
Standardise on options --like-this
379b8c4 
Hyphenate words, and always expect double dashes for long options. A
small number of options have entirely new names.

In general I have added aliases for the unhyphenated and single-dashed
varieties, and used mflag's deprecation syntax to make it print a
warning when it sees them. I left some abbreviated options
unhyphenated (e.g., `bufsz`), and the TLS options as-is since they
correspond with Docker options.

NB the deprecation wanrings will mostly get printed to container logs,
since mostly the executables are run in containers. The next step is
to emit warnings from the script for the options mentioned in the
script usage message.
@squaremo squaremo force-pushed the issue602_moar_hyphens branch from 2b2c892 to 4ede334 Compare July 7, 2015 10:25
squaremo added 2 commits July 7, 2015 11:26
@squaremo
Usage and deprecation warnings in script
ebbb5d3 
The usage is rearranged slightly to avoid wrapping at 80 characters.

This will tend to overestimate warnings, since it doesn't try very
hard to ignore values that are not intended to be options; in
particular, arguments that are intended for `docker run`. (Using `--`
to delimit these arguments would make this easier!)

It does ignore values that follow known options though, for instance
in

        weave -nickname "-password"

(-nickname results in a deprecation warning, -password does not)

and

        weave --password "-iprange"

(no deprecation warning, since -iprange is ignored)
@squaremo
Update docs and smoke-tests with new option names
4ede334
paulbellamy added a commit that referenced this pull request Jul 7, 2015
@paulbellamy
Merge pull request #1084 from squaremo/issue602_moar_hyphens
d39aaa8 
Consistent option naming
@paulbellamy paulbellamy merged commit d39aaa8 into weaveworks:master Jul 7, 2015
@rade rade deleted the issue602_moar_hyphens branch July 8, 2015 16:15
@rade rade modified the milestones: current, 1.1.0 Jul 13, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@paulbellamy paulbellamy

Labels
None yet
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

role of iprange not clear Inconsistent use of - vs -- for long options in weave script
4 participants
@squaremo @rade @bboreham @paulbellamy