Skip to content

Change help commands to use name from scope instead of the name from the declaration#14490

Merged
fdncred merged 1 commit intonushell:mainfrom
132ikl:help-name-scope
Dec 10, 2024
Merged

Change help commands to use name from scope instead of the name from the declaration#14490
fdncred merged 1 commit intonushell:mainfrom
132ikl:help-name-scope

Conversation

@132ikl
Copy link
Copy Markdown
Member

@132ikl 132ikl commented Dec 2, 2024

Description

Before this PR, help commands uses the name from a command's declaration rather than the name in the scope. This is problematic when trying to view the help page for the main command of a module. For example, std bench:

use std/bench
help bench
# => Error: nu::parser::not_found
# => 
# =>   × Not found.
# =>    ╭─[entry #10:1:6]
# =>  1 │ help bench
# =>    ·      ──┬──
# =>    ·        ╰── did not find anything under this name
# =>    ╰────

This can also cause confusion when importing specific commands from modules. Furthermore, if there are multiple commands with the same name from different modules, the help text for both will appear when querying their help text (this is especially problematic for main commands, see #14033):

use std/iter
help iter find
# => Error: nu::parser::not_found
# => 
# =>   × Not found.
# =>    ╭─[entry #3:1:6]
# =>  1│ help iter find
# =>    ·      ────┬────
# =>    ·          ╰── did not find anything under this name
# =>    ╰────
help find
# => Searches terms in the input.
# => 
# => Search terms: filter, regex, search, condition
# => 
# => Usage:
# =>   > find {flags} ...(rest) 
# [...]
# => Returns the first element of the list that matches the
# => closure predicate, `null` otherwise
# [...]
# (full text omitted for brevity)

This PR changes help commands to use the name as it is in scope, so prefixing any command in scope with help will show the correct help text.

use std/bench
help bench
# [help text for std bench]
use std/iter
help iter find
# [help text for std iter find]

use std
help std bench
# [help text for std bench]
help std iter find
# [help text for std iter find]

Additionally, the IR code generation for commands called with the --help text has been updated to reflect this change.

This does have one side effect: when a module has a main command defined, running help <name> (which checks help aliases, then help commands, then help modules) will show the help text for the main command rather than the module. The help text for the module is still accessible with help modules <name>.

Fixes #10499, #10311, #11609, #13470, #14033, and #14402.
Partially fixes #10707.
Does not fix #11447.

User-Facing Changes

  • Help text for commands can be obtained by running help <command name>, where the command name is the same thing you would type in order to execute the command. Previously, it was the name of the function as written in the source file.
    • For example, for the following module spam with command meow:
      module spam { 
          # help text
          export def meow [] {}
      }
      • Before this PR:
        • Regardless of how meow is used, the help text is viewable by running help meow.
      • After this PR:
        • When imported with use spam: The meow command is executed by running spam meow and the help text is viewable by running help spam meow.
        • When imported with use spam foo: The meow command is executed by running meow and the help text is viewable by running meow.
  • When a module has a main command defined, help <module name> will return help for the main command, rather than the module. To access the help for the module, use help modules <module name>.

Tests + Formatting

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • 🟢 toolkit test
  • 🟢 toolkit test stdlib

After Submitting

N/A

@NotTheDr01ds
Copy link
Copy Markdown
Contributor

Sounds great! This has been an issue (and tough to explain in documentation) for a while now.

self.permanent_state.find_decl(name, &removed_overlays)
}

pub fn find_decl_name(&self, decl_id: DeclId) -> Option<&[u8]> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this whole find_decl stuff is a bit confusing and stretched over the different state parts, maybe worth a doccomment what the particular methods are useful for.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I can take a look at adding some here and in engine_state.rs

Comment on lines -91 to -98
.get_decls_sorted(false)
.into_iter()
.filter_map(|(_, decl_id)| {
let decl = engine_state.get_decl(decl_id);
(decl.name() == name).then_some(decl)
})
.map(|cmd| get_full_help(cmd, engine_state, stack))
.collect::<Vec<String>>();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need some separate behavior to find all the possibly now shadowed decls?

Wondering if there is a situation where we want to find more than one like this.

Is this useful if you have multiple same named decls accross modules that are not use'd by name into the scope?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, but I struggle to imagine a scenario where you would actually want to do this.

Is this useful if you have multiple same named decls accross modules that are not use'd by name into the scope?

I'm not quite certain what you mean by this, could you provide an example?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you have two modules used by just there module name like:

use foo.nu
use bar.nu

and want to learn about the command baz defined in both to find out which to import fully

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding, you should be able to still view the help for each:

foo.nu:

# foo spam
export def spam [] {}

bar.nu:

# bar spam
export def spam [] {}
use foo.nu 
use bar.nu 
help foo spam
# => foo spam
# => 
# => Usage:
# =>   > spam 
# => 
# => Flags:
# =>   -h, --help: Display the help message for this command
# => 
# => Input/output types:
# =>   ╭───┬───────┬────────╮
# =>   │ # │ input │ output │
# =>   ├───┼───────┼────────┤
# =>   │ 0 │ any   │ any    │
# =>   ╰───┴───────┴────────╯
# => 
help bar spam
# => bar spam
# => 
# => Usage:
# =>   > spam 
# => 
# => Flags:
# =>   -h, --help: Display the help message for this command
# => 
# => Input/output types:
# =>   ╭───┬───────┬────────╮
# =>   │ # │ input │ output │
# =>   ├───┼───────┼────────┤
# =>   │ 0 │ any   │ any    │
# =>   ╰───┴───────┴────────╯
# => 

@sholderbach
Copy link
Copy Markdown
Member

Thanks for tackling this! That sounds like a great clear up of issues. We need to go through your full list and close when we land :D

@melMass
Copy link
Copy Markdown
Contributor

melMass commented Dec 5, 2024

That one has been bugging me for so long as I use modules for everything!
I just tested it in my rather complex setup and it works as expected thanks a lot!

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Dec 10, 2024

Are we ready to land this?

@132ikl
Copy link
Copy Markdown
Member Author

132ikl commented Dec 10, 2024

I can add the doccomments @sholderbach mentioned in either this PR or a separate PR, so I'm fine with landing this now if there aren't any other concerns. Wrote a note for myself so I don't forget 😄

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Dec 10, 2024

ok, let's do that separately then. Thanks for all your work here.

@fdncred fdncred merged commit 8f9aa1a into nushell:main Dec 10, 2024
@github-actions github-actions bot added this to the v0.101.0 milestone Dec 10, 2024
@132ikl
Copy link
Copy Markdown
Member Author

132ikl commented Dec 10, 2024

hey @fdncred, looks like GitHub picked up the wrong issues to close.

These issues should be closed: #10499 (this did get picked up), #10311, #11609, #13470, #14033, #14402.

This were improperly closed and should be reopened: #10707, #11447.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Dec 10, 2024

ok, thanks. i closed those and re-opened the others.

@Bahex
Copy link
Copy Markdown
Member

Bahex commented Dec 11, 2024

@132ikl Thank you so much for this! I had stupid workarounds because of this issue like prefixing module files with underscores and using the module name instead of main for commands. Feels good to clean that up.

@fdncred fdncred added the deprecated:pr-commands (deprecated: too vague) This PR changes our commands in some way label Dec 18, 2024
WindSoilder pushed a commit that referenced this pull request Jan 6, 2025
#14750)

# Description
Adds some doccomments to some of the methods in `engine_state.rs` and
`state_working_set.rs`. Also grouped together some of the `find` methods
in `engine_state.rs`, but didn't do so in `state_working_set.rs` since
they seem to already be grouped according to decl/overlay/module.

Follow-up to #14490. 

# User-Facing Changes
None

# Tests + Formatting
N/A

# After Submitting
N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deprecated:pr-commands (deprecated: too vague) This PR changes our commands in some way

Projects

None yet

6 participants