Conversation
Summary of ChangesHello @abusch, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the CLI tool by integrating full support for Nushell completions. It allows users of the Nushell environment to generate and leverage intelligent command-line suggestions, streamlining their workflow and improving overall usability. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for nushell completions, which is a great addition. The implementation largely follows the existing patterns for other shells. I've identified a bug related to string escaping in the generated nushell script that could cause issues. I also have a couple of suggestions to improve the documentation and the formatting of the generated script. Overall, this is a solid contribution.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #485 +/- ##
==========================================
+ Coverage 70.71% 70.90% +0.19%
==========================================
Files 47 48 +1
Lines 6716 6796 +80
Branches 6716 6796 +80
==========================================
+ Hits 4749 4819 +70
- Misses 1277 1281 +4
- Partials 690 696 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Remove extra new lines - Switch code block in documentation to `nushell` syntax - Use nushell's raw strings to hold spec, to avoid the need to escape it
abusch
force-pushed
the
nushell_support
branch
from
28479e3 to
9a083b1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for nushell completions. The changes are comprehensive, touching the CLI argument parsing, completion logic, documentation, and adding a new module for generating nushell completion scripts.
Overall, the implementation is well done. I've left a couple of comments on the new lib/src/complete/nu.rs file with suggestions for refactoring to improve code clarity and reduce duplication. These are not critical but would improve the maintainability of the new code.
The rest of the changes, which mostly involve adding 'nu' to the list of supported shells in various files, are correct and consistent. The documentation updates are also clear and helpful.
| let spec_variable = if let Some(cache_key) = &opts.cache_key { | ||
| format!("_usage_spec_{bin_snake}_{}", cache_key.to_snake_case()) | ||
| } else { | ||
| format!("_usage_spec_{bin_snake}") | ||
| }; |
There was a problem hiding this comment.
The way spec_variable is constructed and then used to create spec_file on line 57 leads to a filename with a double underscore (e.g., usage__usage_spec_...). This is a bit awkward and could be improved for clarity.
Consider separating the unique identifier part of the name. This would make the generated filenames cleaner and the code easier to follow.
Example refactoring:
let spec_identifier = if let Some(cache_key) = &opts.cache_key {
format!("{}_{}", bin_snake, cache_key.to_snake_case())
} else {
bin_snake.to_string()
};
let spec_variable = format!("_usage_spec_{}", spec_identifier);
// ... and later in the file:
// let spec_file = $"($nu.temp-dir)/usage_{spec_identifier}.spec"This would produce a filename like usage_mycli_1_2_3.spec instead of usage__usage_spec_mycli_1_2_3.spec.
| let file_write_logic = if let Some(usage_cmd) = &opts.usage_cmd { | ||
| if opts.cache_key.is_some() { | ||
| format!( | ||
| r#"if not ($spec_file | path exists) {{ | ||
| {usage_cmd} | collect | save $spec_file | ||
| }}"# | ||
| ) | ||
| } else { | ||
| format!(r#"{usage_cmd} | collect | save -f $spec_file"#) | ||
| } | ||
| } else if let Some(_spec) = &opts.spec { | ||
| if opts.cache_key.is_some() { | ||
| format!( | ||
| r#"if not ($spec_file | path exists) {{ | ||
| ${spec_variable} | save $spec_file | ||
| }}"# | ||
| ) | ||
| } else { | ||
| format!(r#"${spec_variable} | save -f $spec_file"#) | ||
| } | ||
| } else { | ||
| String::new() | ||
| }; |
There was a problem hiding this comment.
There's duplicated logic for building file_write_logic. The conditional logic for opts.cache_key.is_some() is repeated for both usage_cmd and spec cases. This can be refactored to be more DRY and improve maintainability.
let file_write_logic = {
let spec_source = if let Some(usage_cmd) = &opts.usage_cmd {
Some(format!("{} | collect", usage_cmd))
} else if opts.spec.is_some() {
Some(format!("${}", spec_variable))
} else {
None
};
if let Some(spec_source) = spec_source {
if opts.cache_key.is_some() {
format!(
r#"if not ($spec_file | path exists) {{
{} | save $spec_file
}}"#,
spec_source
)
} else {
format!(r#"{} | save -f $spec_file"#, spec_source)
}
} else {
String::new()
}
};
jdx
left a comment
jdx
left a comment
There was a problem hiding this comment.
Code Review (AI-generated)
Overall this is a solid PR that adds nushell completion support following the established patterns of the existing shell implementations. The changes are well-structured across all the right files. A few issues to flag:
Issues
1. Indentation bug in generated nushell code (medium)
The .trim() call on the format string in nu.rs:52-68 strips the leading 4-space indent from def, causing it to sit at column 0 while everything else inside the module block is properly indented. You can see this in the snapshots:
module _mycli_completions {
def mycli_completer [spans: list<string>] { # <-- should be indented
let spec_file = ...
}
@complete mycli_completer
export extern mycli []
}The const line (when present) is correctly indented at 4 spaces, but def loses its indent. Either remove .trim() and adjust the format string, or restructure to maintain consistent indentation.
2. Doc comment not updated
cli/src/cli/generate/completion.rs:7 still says:
/// Generate shell completion scripts for bash, fish, powershell, or zsh
Should include nu/nushell.
3. Trailing whitespace in generated code
There's a trailing space after the help string in the error message (nu.rs:82):
help: "See https://usage.jdx.dev for more information."
^ trailing space4. Raw string r#'...'# could break on certain specs (low risk)
If a spec ever contains the literal character sequence '#, the nushell raw string r#'...'# would terminate early and produce a syntax error. This is unlikely in practice but worth noting — fish handles a similar concern by escaping single quotes. Could either document this limitation or add validation/escaping.
Looks good
- Grouping
"nu"with"fish" | "powershell"incomplete_word.rsfor tab-separated output is correct and matches the parsing logic in the generated nushell script. - The
@completeattribute +export externpattern is the correct modern nushell completion registration approach. - Module wrapping +
useimport is idiomatic nushell. - Test coverage with 3 snapshot variants matches other shells.
- All plumbing files (fig.ts, KDL spec, docs, commands.json) properly updated.
🤖 This review was AI-generated.
2 participants
Gave a shot at adding support for
nushell.