Rename --temp-directory option to --temp-dir

[AndrewFinlay]

This change makes the option naming consistent using the form --*-dir, fixes #895

Deprecates the option name --temp-directory in favour of --temp-dir. --temp-directory has been demoted to a yargs alias of --temp-dir, so it will still work with existing projects. I've updated the docs and changed the package.json test scripts to use the newer form.

I'm a little unsure of how tests should work for this, I've modified the package.json tests to use the new form, but then that doesn't test that the deprecated form still works, thoughts?

[JaKXz]

I think this makes sense, I like ubiquitous language where we can get it :) thanks for your contribution!

@bcoe or @coreyfarrell I'd push the button but I'll leave it to you two in case you have more yargsish feedback.

[coreyfarrell]

One comment, as is --temp-directory will still be shown in nyc --help. I think this could be hidden by using a separate option for temp-directory with no describe or default, instead of making it an alias for temp-dir. Then when using the option you'd do this._tempDirectory = config.tempDir || config.tempDirectory || './.nyc_output'.

As for actually merging this change, I'd like to wait until we've released nyc@13 as latest. I'm planning for that release to happen early next week.

[AndrewFinlay]

@coreyfarrell - I've run a quick test on the requested changes to the option structure,
the help output goes from:

--temp-dir, --temp-directory directory to output raw coverage information to
[default: "./.nyc_output"]

to

--temp-dir directory to output raw coverage information to
[default: "./.nyc_output"]
--temp-directory

with code:

    .option('temp-dir', {
      describe: 'directory to output raw coverage information to',
      default: './.nyc_output',
      global: false
    })
    .option('temp-directory', {
      global: false
    })

Not sure if this is what you're after.

Is there a better way to handle deprecation in yargs?

[coreyfarrell]

Ah sorry I'm still not very good with yargs, thanks for double-checking my idea. I was looking for the old option to be excluded completely from the help screen. Looks like you need to add hidden: true to the options of the separate temp-directory to do that.

I'm not aware of anything in yargs specifically to help with specifying deprecated aliases, that would be neat if we could add hiddenAlias: 'temp-directory' to the temp-dir option.

[AndrewFinlay]

The hidden flag works well, thanks

[AndrewFinlay]

Regarding yargs, a hiddenDeprecated: 'temp-directory' that shouted a deprecation warning when it sees a 'temp-directory' would be nicer!

[coreyfarrell]

This looks good to me. Sorry for the delay I've been quite busy the past couple weeks. As soon as I have time I'll be publishing nyc@13 to latest on npm, once that's done I'll be ready to merge this.

Thanks for the contribution and addressing my comments!

[coreyfarrell]

Regarding yargs, a hiddenDeprecated: 'temp-directory' that shouted a deprecation warning when it sees a 'temp-directory' would be nicer!

I agree it would be nice if yargs had that kind of support but I feel like silent deprecation is best for this specific option. We'll never reuse temp-directory for any other purpose so the deprecated option can just remain. I don't see any reason to projects by printing an unexpected warning when temp-directory is set.

[jorgebucaran]

@AndrewFinlay @coreyfarrell I just realized that #906 is similar to this PR. Whether you decide to rename --temp-directory to --temp-dir or not, would you please consider adding a short (single-letter) alias to the directive like -t in #906.

[AndrewFinlay]

@coreyfarrell Can add this if needed.

[jorgebucaran]

@AndrewFinlay Could you please go ahead? I'll close my PR then.

[coreyfarrell]

Thanks for the patch @AndrewFinlay, merged.

[simlu]

So this was clearly a breaking change... :( See #927

[AndrewFinlay]

Hi Simlu,

This wasn't intended to be a breaking change, I thought I'd left the old command intact to prevent any breaks but obviously not the case. I'll take a look at it sometime soon, sorry for the inconvenience.

After having a quick look, at a guess I'd say that the default for tempDir is overriding your value for tempDirectory. Will try and put something together tomorrow.

Andrew

[simlu]

@AndrewFinlay Cheers and thank you! Much apreachiated!

[coreyfarrell]

@AndrewFinlay Looking at the patch again I suspect this may be due to our leaving a default value in yargs for temp-dir. This means that the legacy option can never be used because config.tempDir is always set:

  this._tempDirectory = config.tempDir || config.tempDirectory || './.nyc_output'

@simlu As Andrew mentioned the intent was that temp-directory would continue to work (in fact I don't intend to remove it just leave it undocumented).

[AndrewFinlay]

See #928