Fix missing command temp-directory

[AndrewFinlay]

Hi all,

So I've come up with this and added a few tests to confirm behaviour.
Let me know if anything more needs to be done to get this over the line.

Thanks,
Andrew

[coreyfarrell]

One concern I hadn't previously thought of, this will stop yargs from displaying the default value in nyc --help. It's possible that a better fix would be to leave the default in place but modify index.js:

this._tempDirectory = config.tempDirectory || config.tempDir

This would use --temp-directory if specified, --temp-dir otherwise. Removing the || './.nyc_output' at the end would avoid future confusion about how the value is set.

Sorry I didn't think of this at first. @AndrewFinlay @bcoe @JaKXz do you have any opinion on this?

[AndrewFinlay]

It did cross my mind that you'd lose the default setting in the help, but I also thought it to be important to prefer tempDir over tempDirectory.

Looking at the yargs docs there is the opt key defaultDescription, this can output a default description of './.nyc_output' as help text without affecting the value of tempDir. It looks the same in the help text as if a default value were set. This solution is getting a little messy though, and wouldn't do much to clear up confusion about whether the default value is set while building yargs objects or in the index.js file.

So I suppose if preferring tempDir over tempDirectory isn't important we'll go with your solution, otherwise I can add the code to provide defaultDescription.

With a best case solution in mind, and I think we may have discussed it before, adding something like a silentAlias option key to yargs would make what we're trying to do here a whole lot neater.

[coreyfarrell]

I was not aware of the defaultDescription attribute. Unfortunately it looks like that option doesn't work correctly without a truthy value for default. Just setting defaultDescription causes that value to be used as default for tempDir. Even setting default: '' or default: false does not seem to prevent defaultDescription from being used as the actual default.

As for preferring temporaryDir over tempDir this might actually be better. Old versions of nyc --temp-dir were silently ignored. So if both options are specified (silly I know), the old behavior would be to honor --temporary-dir.

[AndrewFinlay]

Sounds good, will move it over to that.
Had a bit more of a look at the tests I proposed and they're not correct so I'll give those another spin.

[coreyfarrell]

@simlu Could you test this updated patch, confirm that it resolves your issue? Thanks!

@AndrewFinlay thanks for your quick responses to this issue!

[coreyfarrell]

Pending feedback from issue reporter.

[JaKXz]

This makes sense! too bad we didn't catch it the first time. Would it be possible to write a test case? 🤷‍♂️

[AndrewFinlay]

This has been on my list for a while now, it hasn't been my highest day job priority but since it's the quiet season I had planned to get onto this next week. Anyway I had a few troubles getting the tests up and running on my first attempt at this and dumped them as they weren't right. So I'll try and get them working properly and submit them with this fix.

It'll need a refactor too as it's a pretty messy commit history so far.

[simlu]

@coreyfarrell This was so breaking for me that I had to fix it immediately - this fix came to late unfortunately. So I'm not really able to test at this point if the old syntax still works as expected - the new one however does the job.

[JaKXz]

Don't worry about the commit history, we'll do a squash when we merge.

@bcoe @coreyfarrell can we just merge this as is and let @AndrewFinlay write a test in a later PR?