Skip to content
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

explore: cleanup params/flags and add more keybindings #1652

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

paulie4
Copy link
Contributor

@paulie4 paulie4 commented Nov 28, 2024

some of the keybindings added to the documentation are existing (i.e. the Vim keybindings), and some of them will be added via nushell/nushell#14468 (i.e. the Emacs keybindings)

@fdncred
Copy link
Collaborator

fdncred commented Nov 29, 2024

The book can be updated but the command docs cannot be updated here. They are autogenerated from the commands themselves. So, if the command help needs to change, the rust code needs to change.

@paulie4
Copy link
Contributor Author

paulie4 commented Nov 29, 2024

Oh, I think the problem is that the make_docs.nu code doesn't check if there's a short_flag so is incorrectly adding , -($param.short_flag) when it shouldn't. What do you think of a change like this?

diff --git a/make_docs.nu b/make_docs.nu
index 5c5a353a..405cd5bf 100644
--- a/make_docs.nu
+++ b/make_docs.nu
@@ -168,13 +168,18 @@ def command-doc [command] {
     }

     let flags = if $no_flags { '' } else {
-        ($command.signatures | get $columns.0 | each { |param|
-            if $param.parameter_type == "switch" {
-                $" -  `--($param.parameter_name), -($param.short_flag)`: ($param.description)"
-            } else if $param.parameter_type == "named" {
-                $" -  `--($param.parameter_name), -($param.short_flag) {($param.syntax_shape)}`: ($param.description)"
+        ($command.signatures | get $columns.0 | each { |param| (
+            try {
+                let start = $' -  `--($param.parameter_name)'
+                let end = $'`: ($param.description)'
+                let short_flag = (if ($param.short_flag | is-empty) {''} else {$', -($param.short_flag)'})
+                if $param.parameter_type == 'switch' {
+                    $'($start)($short_flag)($end)'
+                } else if $param.parameter_type == 'named' {
+                    $'($start)($short_flag) {($param.syntax_shape)}($end)'
+                }
             }
-        } | str join (char newline))
+        ) } | str join (char newline))
     }

     let flags = if $no_flags { "" } else {

@paulie4
Copy link
Contributor Author

paulie4 commented Nov 30, 2024

It looks like @Hofer-Julian was the last person that touched that part of the make_docs.nu file a year ago, and @hustcer added $param.short_flag a year and a half ago, so should they weigh in on my suggested change to add conditionality for printing that flag, since some command parameters don't have short versions?

Should that be in a separate PR? If so, should I undo the commands/docs/explore.md change in this commit so it can be part of the other PR?

@fdncred
Copy link
Collaborator

fdncred commented Nov 30, 2024

yes, you should remove the command docs and @hustcer may be able to chime when he has time.

@paulie4
Copy link
Contributor Author

paulie4 commented Dec 3, 2024

OK, I removed the manual commands/docs/explore.md change from this PR, and this change seems to work for me.

diff --git a/make_docs.nu b/make_docs.nu
index 5c5a353a..a322b570 100644
--- a/make_docs.nu
+++ b/make_docs.nu
@@ -26,7 +26,7 @@ def plugin-paths [ nu_path?: path ] {
 # get all command names from a clean scope
 def command-names [] {
     let plugins = (plugin-paths)
-    nu --no-config-file --plugins ($plugins | to nuon) --commands $'scope commands | select name | to json'
+    nu --no-config-file --commands $'scope commands | select name | to json'
         | from json
 }

@@ -58,7 +58,7 @@ def make_docs [
 ] {
     let $nu_path = ($nu_path | default $nu.current-exe)
     let plugins = (plugin-paths $nu_path)
-    run-external $nu_path "--no-config-file" $"--plugins ($plugins | to nuon)" "make_docs.nu"
+    run-external $nu_path "--no-config-file" "make_docs.nu"
 }

 # generate the YAML frontmatter of a command
@@ -169,10 +169,13 @@ def command-doc [command] {

     let flags = if $no_flags { '' } else {
         ($command.signatures | get $columns.0 | each { |param|
-            if $param.parameter_type == "switch" {
-                $" -  `--($param.parameter_name), -($param.short_flag)`: ($param.description)"
-            } else if $param.parameter_type == "named" {
-                $" -  `--($param.parameter_name), -($param.short_flag) {($param.syntax_shape)}`: ($param.description)"
+            let start = $' -  `--($param.parameter_name)'
+            let end = $'`: ($param.description)'
+            let short_flag = (if ($param.short_flag | is-empty) {''} else {$', -($param.short_flag)'})
+            if $param.parameter_type == 'switch' {
+                $'($start)($short_flag)($end)'
+            } else if $param.parameter_type == 'named' {
+                $'($start)($short_flag) {($param.syntax_shape)}($end)'
             }
         } | str join (char newline))
     }

NOTE: I had to remove the plugins stuff to be able to run the script, since the "C:\\git\\nushell\\target\\debug\\nu_plugin_polars.exe" plugin causes this unhelpful error message, preventing nu from running:

Error: nu::shell::io_error

  × I/O error

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

Feel free to open a PR for the make_docs.nu script!

@sholderbach sholderbach merged commit b0ab1be into nushell:main Dec 3, 2024
2 checks passed
@paulie4
Copy link
Contributor Author

paulie4 commented Dec 3, 2024

OK, I created PR #1658, and I used my Windows WSL Ubuntu, since I couldn't run make_docs.nu with the plugins code when I tried from my Windows (i.e. non-WSL).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants