std help improvements by bobhy · Pull Request #8983 · nushell/nushell

bobhy · 2023-04-24T04:10:32Z

Description

Per #8852, improve layout and usability of std help.
There remains some more work before this is ready for prime time:

Bug: std help bogon doesn't display any error message (expected command_not_found); std help commands bogon displays wrong span in its error.
Add unit tests (after making std help more testable in the first place)
(nice to have) show default value for optional positionals. Right now, default value not in $nu.scope, however.

User-Facing Changes

added std help --all flag (default false; Originally planned to call it --verbose but thought this was more intuitive).
By default, std help command suppresses Category, This Command, Module.

Consolidated usage, flags, signatures and parameters sections into one (hopefully intelligible section.
before:

〉help-old std dirs add
Add one or more directories to the list.
PWD becomes first of the newly added directories.

Category: default

This command:
- does not create a scope.
- is not a built-in command.
- is a subcommand.
- is not part of a plugin.
- is a custom command.
- is not a keyword.

Usage:
 > std dirs add 

Flags:
 -h, --help - Display the help message for this command

Signatures:
 <any> | std dirs add -> <any>

Parameters:
 ...rest <string>: directory or directories to add to working list

after

〉std help std dirs add

Usage: Add one or more directories to the list.
PWD becomes first of the newly added directories.

<any> | std dirs add                    
      ...paths: string                  directory or directories to add to working list
   => <any>

Tests + Formatting

__No tests yet for ``std help`!

After Submitting

codecov · 2023-04-24T04:33:25Z

Codecov Report

Merging #8983 (6cf68ac) into main (bdaa326) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8983   +/-   ##
=======================================
  Coverage   68.72%   68.73%           
=======================================
  Files         640      640           
  Lines      101530   101540   +10     
=======================================
+ Hits        69777    69791   +14     
+ Misses      31753    31749    -4

see 4 files with indirect coverage changes

fdncred · 2023-04-24T11:29:10Z

I'm not understanding scope_hack.

bobhy · 2023-04-24T15:53:25Z

.. and that's why we draft PRs! I'll remove that file.

amtoine · 2023-04-24T17:14:55Z

@bobhy, will this close #8852?

bobhy · 2023-04-24T17:24:54Z

Yes.

MariaSolOs · 2023-04-25T17:20:12Z

crates/nu-std/lib/help.nu

 export def "help commands" [
    ...command: string@"nu-complete list-commands"  # the name of command to get help on
    --find (-f): string  # string to find in command names and usage
+    --all (-a) # if showing help for single command, display all sections


I see that you mentioned this in the PR description, but I think that --verbose is more intuitive. Something like help -a makes me think that I'll get the help for all commands.

For me, --verbose connotes extra output for debugging when the command is misbehaving. How about --full? Whatever we call it, are you OK with the default being less rather than more sections?

I don't think --verbose is tied to debugging only. From my experience it represents a flag saying "apply to all possible arguments".

The thing with --full is that then the short flag would conflict with --find.

And yes, I think that the default should be the shortened version.

That being said, I think it would be a good idea to discuss this with more people in the Discord channel :)

MariaSolOs · 2023-04-25T17:21:30Z

crates/nu-std/lib/help.nu

    } else if not ($command | is-empty) {
        let found_command = ($commands | where name == $command)
-
+        


I think that the check isn't failing because the PR is still a draft, but you should run rustfmt.

crates/nu-command/src/experimental/scope_hack.rs

MariaSolOs · 2023-04-25T17:26:16Z

crates/nu-std/lib/help.nu

    $nu.scope.commands | where is_extern | select name usage | rename value description
 }

+def help-header [


Could you use colorize instead of adding another command?

agree, or help-header could use colorize.

Will use colorize kind of everywhere I can by the time the PR is finalized...

kubouch · 2023-04-27T06:39:43Z

I'm not sure about having --all/--verbose flag on std help commands, it adds a lot of complexity to the help system that I’m not sure is worth it. When a user is seeking help, it needs to be as simple as possible, they shouldn’t need to remember to pass a flag to get some specific information. It also makes the output of cmd --help and help cmd different. I'd vote for having only one level of help messages.

One way to make the std help messages more compact would be to reduce the “This Command” section:

I do not understand what "creates scope" means and what implications it has for the user, could it be omitted?
If a command is built-in, then it is not custom, so the two lines can be combined into one
The information about being a keyword or coming from a plugin could be omitted if false
The subcommand information also does not seem useful to me: You can see it from the command's name and I don’t think user will benefit from that information, the subcommands are just regular commands from Nushell’s point of view.

Also the examples of the std commands are quite lengthy sometimes, it might be possible to make them shorter.

On the other hand, condensing Usage, Flags, Signature and Parameters into one entry, as suggested by this PR, seems too forced and quite confusing to read. To me, the original is more readable.

bobhy · 2023-04-27T18:53:48Z

My feeling is we don't yet cover the most common user help scenario, and some complexity will have to be added somewhere to do so. But maybe I shouldn't be changing the default behavior and should add it elsewhere? Current help is attuned to the "research" scenario. User wants to learn something new, perhaps a deep dive, like "tell me about the `find` command" or a comparison of options, like "what commands are available for dealing with tables", or even conceptual topics like "how do I debug a script". Here the user is focused on getting a comprehensive answer, is expecting a long-form read (multiple pages) and is willing to issue multiple commands, follow multiple links to get a full answer. But other times (and *most* times, sez me) user is asking for help to get a quick reminder on something s/he already knows (or knew), such as "what's the subcommand of `str` that extracts a portion of the string?" or "what's the order of arguments for `cp` command?". Here, user is in the midst of something else and just wants a short answer in a single query. If you buy the second scenario as being valid, I think that justifies a different mode of help display. Current help output tends to show irrelevant detail in this case; and too often, to scroll the relevant portion off the top of the window causing bigger disruption to user flow than is necessary. Independent of the above, I think there will always be differences of opinion on how to format the information, e.g the parameter block and `this command` section. Ultimately, I think these should become themable and configurable (one way might be to define an executable template in `env.nu`, like we do for prompts). For now, I'll back off. How about this as a way forward? * change the sense of the mode flag so it defaults to current style layout, so `--verbose` ==> `--(c)oncise` or `--(q)uick` and put all my compressed display goodies behind it. * revert the parameter block section to its former layout by default. (It isn't bad, just tall) * Rationalize the display under `this command` (which might be better called section `Flags`, see if a horizontal layout works better). * extern, custom, builtin and plugin are, i think, mutually exclusive, so could be combined into 1 line * creates scope does matter for user to know what the effect of a `let` command would be. Though it's weird to think of it as a property of the command, it's more a property of the enclosing brackets (conceptual vs implementation detail; user cares about former). * I can't think of a case where user cares about subcommand status either, agree this should be suppressed. * But keyword and plugin are properties that might matter to user: keyword as a warning that syntax may not be simple, plugin to trace down provenance of the command. So I think we should continue to show the value of the flag for all commands. In default mode, we're not so space constrained.

…

On Thu, Apr 27, 2023 at 2:39 AM Jakub Žádník ***@***.***> wrote: I'm not sure about having --all/--verbose flag on std help commands, it adds a lot of complexity to the help system that I’m not sure is worth it. When a user is seeking help, it needs to be as simple as possible, they shouldn’t need to remember to pass a flag to get some specific information. It also makes the output of cmd --help and help cmd different. I'd vote for having only one level of help messages. One way to make the std help messages more compact would be to reduce the “This Command” section: - I do not understand what "creates scope" means and what implications it has for the user, could it be omitted? - If a command is built-in, then it is not custom, so the two lines can be combined into one - The information about being a keyword or coming from a plugin could be omitted if false - The subcommand information also does not seem useful to me: You can see it from the command's name and I don’t think user will benefit from that information, the subcommands are just regular commands from Nushell’s point of view. Also the examples of the std commands are quite lengthy sometimes, it might be possible to make them shorter. On the other hand, condensing Usage, Flags, Signature and Parameters into one entry, as suggested by this PR, seems too forced and quite confusing to read. To me, the original is more readable. — Reply to this email directly, view it on GitHub <#8983 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPBIZH3IZIXF7TQ6U4BP6DXDIIDVANCNFSM6AAAAAAXJAMKRY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

bobhy · 2023-04-28T06:36:05Z

This is worth playing with, would appreciate interim feedback.
Don't worry about detailed diffs. It will have to be reviewed as a total rewrite (but it's just one file).
See todo's at top of help.nu for current punch list.
Switched to --concise, did 1 of 2 layout changes for help command. So non-default --concise is my innovative view, working only for commands right now. default view will look mostly like builtin help.
help main command works for all scenarios (maybe?) but help commands, help modules etc do not.

for help commands, add --all to suppress less essential info by default; combine usage, signature and params into one glorious table

… you rename the file and don't change the subcommand name. Fiddle syntax of positional parameters.

--all ==> --verbose for more details

Removing unintended extra file from PR

…ation)

for help commands, add --all to suppress less essential info by default; combine usage, signature and params into one glorious table

… you rename the file and don't change the subcommand name. Fiddle syntax of positional parameters.

Removing unintended extra file from PR

items is regex, unanchored for --find, but whole word match for command lookup 'this command' -> Flags section in command

…ut is slow!

bobhy · 2023-05-01T05:32:44Z

Almost ready for review, please ignore all the diffs and just install the one modified file to give it a try.

Performance is slow (on my system, at least), I think due to terminal ANSI emulation (Gnome terminal on Debian). How is it for you reviewers?

Command display for signature, parameters, etc not yet reverted to builtin help. Note that display shows 1 group of flags, params for all signatures. But it's possible for each signature to have different flags and params (though few bultins actually do.

Right now help <unknown> and help commands <unknown> return an empty list rather than raise an error. .Seems a bit less user-hostile... Feedback?

fdncred · 2023-05-01T13:00:48Z

trying to figure out what to test. looking at the diff it's not intuitive.

std help - looks fine
std help bogon - i would prefer an error message when a command isn't found
std help commands bogon - same as above
std help dirs add - does nothing for me
std help aliases - looks ok
std help modules - looks ok
perf isn't very good. sometimes command runs find but it takes a while to redraw the prompt.

These were working and are not now.

std help aliases -f z - doesn't highlight results
std help modules -f weather - doesn't highlight results
std help operators -f math - produces nothing when it should highlight all the maths
std help commands -f git - finds the wrong results
std help externs - all double spaced, -f doesn't find results or highlight

if there's some specific commands that need testing, please indicate it.

amtoine · 2023-05-01T14:53:12Z

@fdncred
for the things not working properly, maybe i've addressed them on latest main but if that branch is based on an older revision, you won't have the fixes and it will appear this PR "broke" them 😮

after pulling latest main, you might want to git checkout -b my_test, git merge main, try the PR and once you're done, git checkout <feature-branch> and git branch --delete --force my_test 😋

…examples; --find that matches exactly 1 now shows item, not list

fdncred · 2023-05-01T15:27:59Z

but if that branch is based on an older revision, you won't have the fixes

right, that was my point. it's important to keep changes on the latest main because fixes are landed frequently. this also making having a pr with a lot of changes difficult. this is also one of the reasons why i was saying we should break up these scripts into smaller bits. now that kubouch has a draft pr where we can "use" a folder, maybe that will help too?

bobhy · 2023-05-01T15:53:00Z

Thanks for taking a look. I am keeping up with main as recently as this AM.
I'll pull in the use folder changes and refactor help to suit. Once it's there, the code base will be stable enough to review diffs, but, for now, just consider it's a total rewrite each time, ignore the diffs and just try running the updated thing. @amtoine If you could keep the velocity of changes down for existing help.hu in main, that'll make the refactoring go faster.

lack of highlighting for --find results -- this is because find --regex doesn't highlight matches.

bobhy · 2023-05-01T16:11:03Z

.. Would also appreciate feedback from y'all on screen-painting perf. std help --find is ok, but std help <command> is noticeably slow, but maybe that's just me (and my Debian Gnome terminal)...

fdncred · 2023-05-01T16:25:41Z

lack of highlighting for --find results -- this is because find --regex doesn't highlight matches.

imo, we need to use | find -c [<column names to search>] so we get the searching and highlighting only in certain columns. If the user wants/needs to use regex, they should specify it. The same sentiment for all the parameters of find. I'd really prefer to not reinvent/reinterpret help and find right now. What I'd like to shoot for is a mirror image of how it works in rust code first. Then, later, if we want to change it, we can talk about doing that.

amtoine · 2023-05-01T17:24:11Z

the find issue has been tackled in FIX: filter the std help ... by name, usage and search terms #9035
the slowness of std help is tracked in bounty: divergences between built-in help and the implementations in nu-std #8813

MariaSolOs · 2023-05-01T17:33:01Z

crates/nu-std/lib/help.nu

+def print-help-section [section:string,  # section name, skip printing if ''
+                        body?:string = ""


Nit:

Suggested change

def print-help-section [section:string, # section name, skip printing if ''

body?:string = ""

def print-help-section [

section: string, # section name, skip printing if ''

body?: string = ""

Agree the formatting is not standard. Wish there were a nufmt tool that would handle this automatically.

I believe @amtoine is working on it 😉

MariaSolOs · 2023-05-01T17:33:33Z

crates/nu-std/lib/help.nu

+    if not ($body | is-empty) {
+        if $section != "" {
+            print-help-header $section
+        } else {print ""}


Nit:

Suggested change

} else {print ""}

} else {

print ""

}

MariaSolOs · 2023-05-01T17:44:47Z

crates/nu-std/lib/help.nu

-    ...command: string@"nu-complete list-commands"  # the name of command to get help on
-    --find (-f): string  # string to find in command names and usage
+    ...item: string@"nu-complete list-commands"  # the name of command to get help on
+    --find (-f)     # show list of matches


Question: Could the find pattern used here be based on aliases?

Do you mean, could I specify an alias here and get help for the command name in the expansion of the alias? If so, no.
I just noticed that built-in help does do this. I actually don't think it should, it's hiding too much information. User typed in an alias, help should tell him/her about that alias.

Yeah, that's what I meant. I don't have strong feelings about whether it should or should not be supported, but at least it should match what built-in help does.

bobhy · 2023-05-01T20:30:08Z

@fncred -- this version is already using find --columns to limit the columns searched, but the choice of columns for each kind could be improved.

As for replicating in stdlib exactly what we already have in the builtin help, I don't see the point of working on that project. Why would we spend time/effort building and maintaining that?

What I've done so far on the current PR convinces me Nu language and stdlib are a sufficiently powerful tool to build an improved version of help, and we should be having the discussion about what those improvements should be.

fdncred · 2023-05-02T11:59:49Z

After the discussion with @bobhy yesterday in Discord https://discord.com/channels/601130461678272522/615329862395101194/1102698811312320532, it appears that Bob won't be working on this PR anymore. I'm fine with people working where their passion is. So, we have two options.

Someone picks up and finishes this PR or we close it.

I've done some testing this morning on my mac and it seems that real-world performance is better here. I'm assuming that's because of the par-each's but I'm not really sure. One of the thing we've been trying to figure out is performance. So that's a +1 for this PR.

I've also noticed that there are some regressions for things like help operators. The current main more closely matches our existing help operators written in rust.

I really think, one of the big problems is that help.nu is big which makes it more difficult to doing small, bit-sized changes, which in turn may make it more difficult to work on and also makes it more difficult to review.

In order to move forward with this PR, I feel like what needs to be done is 100% every help command needs to be tested to see where it's at. An alternate approach is to just steal the par-each parts and put them in a new PR that only addresses performance. That may be the easier approach.

What do you think we should do?

bobhy · 2023-05-02T12:19:12Z

@fncred In my defense, I do have passion for improving the help "system" in Nu and am working on #8928 right now. I want to continue by splitting help -f out into a separate apropos -like command, that will simplify main help. And I'll continue to polish an alternative (improved?) presentation for help info. We can decide later what to call it and how to ship it.

amtoine · 2023-05-02T17:57:41Z

📯 announcement

there are great ideas in this PR but a few things make it hard to land right now...

there is no more contributor to finish it
the PR grew silently due to its DRAFT status and is now a bit hard to review
there is no significant speed boost
some regressions have been seen
there are some broken parts, e.g. when running std help with --no-config-file

however, this is not the end of these changes!

i've linked this PR in #9061, which is easily reachable from the roadmap for future contributors to take the lead on this again 😌

thanks @bobhy for the efforts, @MariaSolOs for reviewing this a bit and all the other members of this discussion.
looking forward to seing the style improvements from this PR in the future 🥳

bobhy marked this pull request as draft April 24, 2023 04:10

fdncred added the A:std-library Defining and improving the standard library written in Nu label Apr 24, 2023

amtoine self-requested a review April 24, 2023 17:12

amtoine assigned bobhy Apr 24, 2023

amtoine linked an issue Apr 24, 2023 that may be closed by this pull request

Improve layout of help output, and autogenerated help for custom commands #8852

Open

MariaSolOs reviewed Apr 25, 2023

View reviewed changes

bobhy force-pushed the help_help branch from a082edc to bbf73a4 Compare April 25, 2023 23:26

bobhy mentioned this pull request Apr 26, 2023

Usage string for aliases #8999

Open

MariaSolOs mentioned this pull request Apr 27, 2023

help command should include default parameter values #8928

Closed

bobhy force-pushed the help_help branch from bf62b4e to cec8399 Compare April 28, 2023 06:31

bobhy added 12 commits April 30, 2023 09:40

wip commit

251c075

remove extra layer of help subcommand;

10ff1b1

for help commands, add --all to suppress less essential info by default; combine usage, signature and params into one glorious table

revert dropping 'help' from all subcommands. It's only redundant when…

dd87242

… you rename the file and don't change the subcommand name. Fiddle syntax of positional parameters.

wip commit

f2727e4

use colorize where possible;

959d732

--all ==> --verbose for more details

Delete scope_hack.rs

ffcc54a

Removing unintended extra file from PR

lead rhs with extra blank if lhs overflows (e.g for long record annot…

e7826b0

…ation)

wip commit

31a9fac

remove extra layer of help subcommand;

78d04b3

for help commands, add --all to suppress less essential info by default; combine usage, signature and params into one glorious table

revert dropping 'help' from all subcommands. It's only redundant when…

931fd3c

… you rename the file and don't change the subcommand name. Fiddle syntax of positional parameters.

wip commit

f0648a8

Delete scope_hack.rs

ac4c082

Removing unintended extra file from PR

bobhy added 2 commits April 30, 2023 09:40

--concise not --verbose;

ff704c1

items is regex, unanchored for --find, but whole word match for command lookup 'this command' -> Flags section in command

everything works; command signature display needs work; terminal outp…

9180b96

…ut is slow!

bobhy force-pushed the help_help branch from cec8399 to 9180b96 Compare May 1, 2023 05:20

errors for command, module... not found; remove extra blank lines in …

6cf68ac

…examples; --find that matches exactly 1 now shows item, not list

MariaSolOs reviewed May 1, 2023

View reviewed changes

amtoine closed this May 2, 2023

bobhy deleted the help_help branch August 25, 2023 04:22

		} else if not ($command \| is-empty) {
		let found_command = ($commands \| where name == $command)

		def print-help-section [section:string, # section name, skip printing if ''
		body?:string = ""

Conversation

bobhy commented Apr 24, 2023

Description

User-Facing Changes

Tests + Formatting

After Submitting

Uh oh!

codecov bot commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fdncred commented Apr 24, 2023

Uh oh!

bobhy commented Apr 24, 2023

Uh oh!

amtoine commented Apr 24, 2023

Uh oh!

bobhy commented Apr 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MariaSolOs Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kubouch commented Apr 27, 2023

Uh oh!

bobhy commented Apr 27, 2023 via email

Uh oh!

bobhy commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobhy commented May 1, 2023

Uh oh!

fdncred commented May 1, 2023

Uh oh!

amtoine commented May 1, 2023

Uh oh!

fdncred commented May 1, 2023

Uh oh!

bobhy commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobhy commented May 1, 2023

Uh oh!

fdncred commented May 1, 2023

Uh oh!

amtoine commented May 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobhy commented May 1, 2023

Uh oh!

fdncred commented May 2, 2023

Uh oh!

codecov bot commented Apr 24, 2023 •

edited

Loading

MariaSolOs Apr 25, 2023 •

edited

Loading

bobhy commented Apr 28, 2023 •

edited

Loading

bobhy commented May 1, 2023 •

edited

Loading