Fix .version() API inconsistency

[maxrimue]

This pull request changes the version() API to solve the inconsistency as raised by @bcoe in #329

[bcoe]

I think we can simply this logic a bit, and avoid the ternary operator. I might be tempted to do something like this:

1. check the # of arguments, if there's only 1 assume that it represents the version #.
2. if there are 2 assume that it's opt, version.
3. if there are 3 assume opt, description, version.

self.version = function (opt, desc, version) {
  if (arguments.length === 1) {
    version = opt
    opt = 'version'
  } else if (arguments.length === 2) {
    version = desc
    desc = null
  }

  desc = desc || usage.deferY18nLookup('Show version number')
  usage.version(version)
  self.boolean(opt)
  self.sescribe(opt, desc)
}

[maxrimue]

@bcoe I absolutely agree with this idea, to be honest, the only reason why I kept it this short was because the code before seemed rather valuing shortness either, I would like to make it more readable as well

[bcoe]

@maxrimue 👍 this is what code review is all about, nothing to feel bad about (mind you, I definitely have a guilt complex about some of the terrible code I've added to yargs 😛 ).

[maxrimue]

@bcoe Well it does its job doesn't it? :D I was thinking about that we could maybe create a general global function for this parameter thing though, it would make sense if there are some other functions that need to check first what parameters are provided etc., so we can write code like this:

self.version = function (opt, desc, version) {
  var args = getParameters([opt, desc, version], ['opt', 'desc'], ['version']);
  usage.version(args.version)
  self.boolean(args.opt)
  self.describe(args.opt, args.desc)
}

The function call passes the parameters, tells which parameters are optional and which required, and then returns an object with the same names or so, was just a random idea though.

[nexdrew]

How do y'all feel about defaulting the version value to the parent package.json value (if found)? It has always bugged me that I have to code this manually each time, and I always end up doing the same thing. Am I just missing something?

[maxrimue]

@nexdrew Well, you could create a function that finds out that value itself and use it for .version(), but you're right there, there could be a function that does exactly that as a default value for the version

[nexdrew]

I always just want to customize the option (usually -v with --version alias) and it's annoying to require(../package.json).version every time. (Though we probably shouldn't use require() internally for this, to avoid the dynamic require problem.)

[bcoe]

@maxrimue this is looking great, one thing I noticed ... this means that:

.default(key, value, [description])

Is inconsistent with the rest of the API too ... description should probably be the second value right?

[bcoe]

testing GitHub integration, CC: @nexdrew, @Irina perhaps you could give @maxrimue some feedback too.

[maxrimue]

@bcoe Yes, I think so too, description should be the second value as some other functions also accept descriptions as their second parameters

[lrlna]

I think I need a bit more time with the codebase before I make reasonable code comments. Give me a bit more time ^.^

Did have a question about the reason for keeping description as second argument. I've normally seen args "snowball" their way down; e.g. :

• if 1 arg -- opt
• if 2 arg -- opt, version
• if 3 arg -- opt, version, desc
• etc etc etc

Do we just have other pieces of code taking description as 2nd ?

[maxrimue]

@lrlna Quoting @bcoe:

The reason we've been standardizing on description as the second argument, is that there are a few cases where the third argument can be a function. So your code looks most clean if the function lands as the last argument.

[lrlna]

@maxrimue cool -- thanks for clarifying!

[maxrimue]

@bcoe I made it more readable now, what do you think? Is the else path maybe too much?

[nexdrew]

I would make the last part its own sentence:

`version` parameter) and exits the process. If not provided, it will default to `--version`.

[nexdrew]

I like this.

But, just to get this off my chest, it feels wrong to make this breaking change for such a trivial thing in the name of consistency - without a way to deprecate or account for the old usage. I mean, we could just add another method, though that would leave the API inconsistency intact. Argh!

Ok, now that that's out of the way, carry on. 👍

[nexdrew]

As this represents the first breaking change to the API, I'd like to suggest that we add a new section to the CHANGELOG that documents known breaking changes b/w 3.x and 4.x, with notes on how to upgrade.

Maybe something like this:

4.x Breaking Changes

• PR Fix .version() API inconsistency #330: .version() method now expects the version argument to be last instead of first.

  Example: change .version('1.0.0', 'v', 'Print app version') to .version('v', 'Print app version', '1.0.0')

[bcoe]

I'm pretty sure you can get away with just making this:

self.version = function (ver) {
  version = ver
}

[bcoe]

@maxrimue this is looking great, just a couple more comments -- we should also make sure to open up a ticket to track @nexdrew's suggestion of defaulting to the version in package.json. This might be a great next ticket for you to tackle @maxrimue 😄

[bcoe]

@nexdrew I agree, but how do you feel about landing a bunch of our proposed breaking changes on the 4.x branch -- move forward in one slightly painful jump.

[nexdrew]

@bcoe Yes, I totally agree we should be merging these changes to the 4.x branch. I just want to make sure we document the known breaking changes so, when we start to publish 4.x versions, our users have a clear migration path. E.g. "here's what we changed" and "here's what you need to do to upgrade".

[nexdrew]

Not sure if we have a mantra or guiding principle for 4.x, but in my eyes it should be something like:

Make the existing features easier, less verbose, more consistent. And add some new cool features.

Quite possible that's too obvious or simplistic, but having a "statement of objective" might help us measure the value of ideas.

E.g. if this is something I can do in 3.x, is it now "easier" (to do or understand), "less verbose" (requires fewer keystrokes), or "more consistent" (works the same way as other parts of the API) in 4.x? If the answer is yes to 2/3 or greater, then adopt. If 1/3 or less, then drop it.

Granted, the answers will not always be a clear "yes" or "no", and they may heavily depend upon the use-cases, but a warped yard stick is better than no yard stick, right?

[bcoe]

I think it might be worth adding one more test, for:

.version('version', '1.0.0');

accepts version option as first argument, and version number as second argument.

We'll eventually add additional tests for:

.version()

// and

.version('version')

[bcoe]

Great work @maxrimue.