-
-
Notifications
You must be signed in to change notification settings - Fork 359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename --temp-directory option to --temp-dir #897
Conversation
Deprecates the option name --temp-directory in favour of --temp-dir --temp-directory has been demoted to a yargs alias of --temp-dir This change makes the option naming consistent using the form --*-dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 yargs
ish feedback.
One comment, as is --temp-directory will still be shown in As for actually merging this change, I'd like to wait until we've released nyc@13 as |
@coreyfarrell - I've run a quick test on the requested changes to the option structure,
to
with code:
Not sure if this is what you're after. Is there a better way to handle deprecation in yargs? |
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 I'm not aware of anything in yargs specifically to help with specifying deprecated aliases, that would be neat if we could add |
The hidden flag works well, thanks |
Regarding yargs, a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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 |
@AndrewFinlay @coreyfarrell I just realized that #906 is similar to this PR. Whether you decide to rename |
@coreyfarrell Can add this if needed. |
@AndrewFinlay Could you please go ahead? I'll close my PR then. |
Oops, I missed the silent deprecation of temp-directory in the report and merge commands, fixed now.
Thanks for the patch @AndrewFinlay, merged. |
So this was clearly a breaking change... :( See #927 |
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 |
@AndrewFinlay Cheers and thank you! Much apreachiated! |
@AndrewFinlay Looking at the patch again I suspect this may be due to our leaving a default value in this._tempDirectory = config.tempDir || config.tempDirectory || './.nyc_output' @simlu As Andrew mentioned the intent was that |
See #928 |
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?