Help improvements #398
Help improvements #398
Conversation
| @@ -206,6 +207,23 @@ describe('yeoman.generators.Base', function () { | |||
| required: true | |||
| }); | |||
| }); | |||
|
|
|||
| it('should not raise an error if required arguments are not provided, but the help option has been specified', function () { | |||
| var dummy = new generators.Base([], { | |||
There was a problem hiding this comment.
Here you can use helpers.createDummyGenerator() method.
There was a problem hiding this comment.
I don't see how that would simplify things. Wouldn't I still have to generate an instance of the dummy generator? Maybe I simply don't understand how to properly use helpers.createDummyGenerator().
There was a problem hiding this comment.
I jumped to fast to conclusion, the way you do it here is correct.
|
I like the idea and I really like you added many tests to cover the correct behavior. Thanks a lot for the time you put in this! |
|
Thanks for the comments, lots of good feedback! I'll take a look at this later and refactor to address your comments. |
|
Started work on some of these, will commit an updated version later. |
|
I've addressed most of the comments. Let me know if this works. Still struggling a bit with the Let me know if you want the commits squashed into a single commit. |
|
Coverage increased (+0.16%) when pulling adcafe941daca011ead62ae07bcc3dff44c08394 on nwinkler:help-improvements into f9fa9f9 on yeoman:master. |
|
Let me know if you have additional comments or want all of the commits squashed into a single one. |
|
Thanks for your work on addressing our comments @nwinkler. If you could squash your commits into one that would be amazing. |
| @@ -475,6 +501,14 @@ Base.prototype.usage = function usage() { | |||
| return out; | |||
| }; | |||
|
|
|||
| var formatArg = function formatArg(argItem) { | |||
There was a problem hiding this comment.
It'd be better to use a function declaration here. This way the function is hoisted and usable anytime. With var declaration, formatArg is undefined until line 503 is runned. It works now because the code is asynchronous, but this is error prone.
Plus there's quite a lot of concat going on in there. Pretty sure this can be refactored to only use strings + concatenation and remove the duplicated conditional.
There was a problem hiding this comment.
Broke it down into a single conditional and + concatenation instead of concat calls.
|
Added some reviews, otherwise it looks good and almost ready to be merged. Thanks a lot for the time you put on this! |
|
Coverage remained the same when pulling ed460c0844abe9256c73a3547a8243789f6fb224 on nwinkler:help-improvements into f9fa9f9 on yeoman:master. |
|
Coverage increased (+0.16%) when pulling 4cf2a974110340ffc70b760fa43a4383f69376db on nwinkler:help-improvements into f26ae33 on yeoman:master. |
|
Thanks for the additional comments, I've learned a lot again. I've addressed most of them and squashed everything into a single commit. Let me know if you have further comments. This was fun, thanks for being patient with me! |
|
Hey, just need to cleanup last thing and it'll be ready to merge. |
Previously, running something like `yo angular:directive --help` would only show an error due to the missing required _name_ argument. This was not very helpful for the user. I’ve changed this so that when providing the help option, the required arguments are added, but no error is raised if the argument is not provided. This results in a clean and consistent handling of the help option. Now when running `yo angular:directive --help`, the standard help/usage screen is shown, listing the options and arguments. In addition to that, I’ve enhanced the help/usage function to list the arguments both in the usage line and also as an additional table similar to the options table. Let me know if that format is OK to keep like that, I tried to keep it similar to format used by the options. As a byproduct, I had to add the _namespace_ to the _options_ object in `env.create`, since it’s used for displaying the correct name of the generator in the usage line. With the previous version, a call to `yo angular:directive --help` resulted in the usage line `yo directive`. Using the namespace in this place ensures that the correct name is displayed in the usage line.
|
Not sure how that one slipped in - it should be fixed now. Sorry about that. |
|
Awesome! Thanks a lot for the hard work |
|
Thanks, I really enjoyed this! |
Previously, running something like
yo angular:directive --helpwould only show an error due to the missing required name argument. This was not very helpful for the user.I’ve changed this so that when providing the help option, the required arguments are added, but no error is raised if a required argument is not provided. This results in a clean and consistent handling of the help option. Now when running
yo angular:directive --help, the standard help/usage screen is shown, listing the options and arguments.In addition to that, I’ve enhanced the help/usage function to list the arguments both in the usage line and also as an additional table similar to the options table. Let me know if that format looks good, I tried to keep it similar to the format used by the options.
As a byproduct, I had to add the namespace to the options object in
env.create, since it’s used for displaying the correct name of the generator in the usage line. With the previous version, a call toyo angular:directive --helpresulted in the usage lineyo directive. Using the namespace in this place ensures that the correct name is displayed in the usage line:yo angular:directive.