Add noman-help-format to specify argument ordering #10
Conversation
|
Thanks for the contribution @nverno 🙏 This looks like a great idea! Give me a couple of days to give it a thorough review, and I'll get back to you 🙂 |
There was a problem hiding this comment.
Thanks again for the contribution @nverno ! 🎊
I like this idea a lot; it makes the somewhat hard-coded assumption that --help or help are the only possible help suffixes a lot more flexible 👍
However, I tried running it for npm and Go locally, and found that the subcommands weren't correctly parsed as buttons 🤔
Perhaps you could add some test cases for these commands with example output that should be correctly parsed? 🙏
Hopefully you'll find the existing test cases easy to extend, but if not I'd be happy to help write the test cases 🙂
| @@ -94,36 +105,28 @@ If set, this value will used when displaying help for shell built-in commands." | |||
| (let ((subcommand (button-label button))) | |||
| (noman (format "%s %s" noman--last-command subcommand)))) | |||
|
|
|||
| (defun noman--make-aws-button (line) | |||
| (defun noman--make-aws-button (&rest _) | |||
There was a problem hiding this comment.
Why do these functions need to take &rest arguments? It doesn't look like we need to pass any arguments to them any more, and they're not using the _ argument. Could they be parameterless?
There was a problem hiding this comment.
However, I tried running it for npm and Go locally, and found that the subcommands weren't correctly parsed as buttons 🤔
Did you reevaluate noman-parsing-functions to make sure it was using the functions for npm/go? Those commands are buttonized when I run them locally, Ill try to add some tests.
There was a problem hiding this comment.
Why do these functions need to take
&restarguments? It doesn't look like we need to pass any arguments to them any more, and they're not using the_argument. Could they be parameterless?
You're right, that was leftover from when I was experimenting... I was wondering if they should take a context argument - something that could hold parsing state.
For example, if the parsing function might want to only buttonize matches in a certain part of the help output, eg. after seeing a line like "Additional commands:" or something. So the parsing function could have a state like context.in_commands = true.
WDYT?
There was a problem hiding this comment.
I think that's a good idea for the future, and I like your subcommand-p idea 👍
However, if we're not using that argument just now, I think it would be better to make those functions parameterless. We can always introduce a parameter in the future when we need to (doing so would certainly make the aws parsing more accurate).
There was a problem hiding this comment.
Oh wait, I've just re-read your implementation, and I misunderstood it the first time round.
Yes, this is a good idea, let's keep it in 🙂
9b8f8a3 to
25cafd7
Compare
|
When adding a test for the npm parser I realized that the buttons should be parsed differently on the main help page vs. the subcommand pages. So, I added an argument LMK if that seems like an OK thing to do. |
| @@ -94,36 +105,28 @@ If set, this value will used when displaying help for shell built-in commands." | |||
| (let ((subcommand (button-label button))) | |||
| (noman (format "%s %s" noman--last-command subcommand)))) | |||
|
|
|||
| (defun noman--make-aws-button (line) | |||
| (defun noman--make-aws-button (&rest _) | |||
There was a problem hiding this comment.
I think that's a good idea for the future, and I like your subcommand-p idea 👍
However, if we're not using that argument just now, I think it would be better to make those functions parameterless. We can always introduce a parameter in the future when we need to (doing so would certainly make the aws parsing more accurate).
| @@ -141,6 +141,45 @@ fi | |||
| (message name) | |||
| name)) | |||
|
|
|||
| (defun make-npm () | |||
There was a problem hiding this comment.
These tests look good, thanks for adding them 🙏
Thanks for adding the test case for This PR looks good to me; I think my initial problem with |
There was a problem hiding this comment.
@nverno looks good to me, I think you just have a minor linting failure in the CI 🙏
Also adds parsers for npm and go which both use the format <cmd> help <subcommand> for further help on subcommands.
What do you think about adding a way to specify how the help commands
are built for a given command?
Here, I've added a
noman-help-formatvariable and additional parsers fornpmandgowhich both use the format<command> help <subcommand>for additional help.I also simplified (I think) the parsing functions - they can return lists of (beg . end) instead of making buttons and don't take a line argument. I think they could be simplified further to optionally just be regexps, where a match would become a button.
LMK if you're interested in any of the changes and I can modify the pull request.