Changeset release master - #185
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Reviewer's GuideImplements performance and robustness improvements for Sequence diagram for fnm env --use-on-cd initial version applicationsequenceDiagram
actor User
participant Shell
participant FnM as FnmEnvCommand
participant Config as FnmConfig
participant UseCmd as UseCommand
User->>Shell: run fnm env --use-on-cd --shell <S>
Shell->>FnM: execute Env::apply(config)
FnM->>Config: clone()
FnM->>FnM: compute multishell_path
FnM->>FnM: set_path_for_multishell(multishell_path)
FnM->>Config: with_multishell_path(multishell_path)
FnM->>UseCmd: construct Use { version: None, install_if_missing: false, silent_if_unchanged: true, info_to_stderr: true }
FnM->>UseCmd: apply(&config_with_multishell)
UseCmd->>UseCmd: detect current directory version
UseCmd->>UseCmd: maybe install version
UseCmd->>UseCmd: maybe switch multishell to directory version
UseCmd-->>FnM: Result (ignored)
FnM-->>Shell: print shell.use_on_cd(config)
Shell-->>User: evaluates hook for future cd events
Sequence diagram for Windows CMD cd hook with quoted macro pathsequenceDiagram
actor User
participant Cmd as WindowsCmdShell
participant FnM as WindowsCmdImpl
participant Config as FnmConfig
User->>Cmd: run fnm env --use-on-cd --shell cmd
Cmd->>FnM: WindowsCmd.use_on_cd(&config)
FnM->>Config: get_base_dir()
FnM->>FnM: create_cd_file_at(base_dir/cd.cmd)
FnM-->>Cmd: doskey cd="<path_to_cd.cmd>" $*
Cmd-->>User: future cd uses quoted macro (supports paths with spaces)
User->>Cmd: cd C:\Path With Spaces
Cmd->>Cmd: executes cd.cmd with /d %*
Cmd->>FnM: fnm use --silent-if-unchanged
FnM-->>Cmd: adjusts Node version for directory
Class diagram for updated Use command and configuration typesclassDiagram
class FnmConfig {
<<struct>>
+Url node_dist_mirror
+PathBuf base_dir
+Option~PathBuf~ multishell_path
+with_base_dir(base_dir Option_PathBuf) FnmConfig
+with_multishell_path(multishell_path PathBuf) FnmConfig
}
class Directories {
<<struct>>
+Directories(basedirs BaseStrategy)
}
class Use {
<<struct>>
+Option~UserVersion~ version
+bool install_if_missing
+bool silent_if_unchanged
+bool info_to_stderr
+apply(config FnmConfig) Result~(), Error~
}
class EnvCommand {
<<struct>>
+bool use_on_cd
+apply(config FnmConfig) Result~(), Error~
-set_path_for_multishell(multishell_path Path)
}
class Command {
<<trait>>
+apply(config FnmConfig) Result~(), Error~
}
FnmConfig "1" <-- "1" Directories : uses
Command <|.. Use
Command <|.. EnvCommand
class install_new_version_fn {
<<function>>
+install_new_version(requested_version UserVersion, config FnmConfig) Result~(), Error~
}
class use_installed_version_fn {
<<function>>
+use_installed_version(version Version, config FnmConfig) Result~(), Error~
}
install_new_version_fn ..> Use : constructs
use_installed_version_fn ..> Use : constructs
EnvCommand ..> Use : constructs
EnvCommand ..> FnmConfig : with_multishell_path
EnvCommand ..> Directories : uses
Class diagram for updated Arch enumclassDiagram
class Arch {
<<enum>>
X86
X64
X64Musl
X64Glibc217
Arm64
Armv7l
Ppc64le
Aarch64AppleDarwin
fn as_str() str
fn from_str(s str) Result~Arch, Error~
}
Class diagram for shell-specific use-on-cd behavior (Zsh and WindowsCmd)classDiagram
class Shell {
<<trait>>
+env(config FnmConfig) Result~String, Error~
+use_on_cd(config FnmConfig) Result~String, Error~
}
class Zsh {
<<struct>>
+use_on_cd(config FnmConfig) Result~String, Error~
}
class WindowsCmd {
<<struct>>
+env(config FnmConfig) Result~String, Error~
+use_on_cd(config FnmConfig) Result~String, Error~
}
Shell <|.. Zsh
Shell <|.. WindowsCmd
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello, 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 focuses on a series of enhancements to Highlights
Changelog
Ignored Files
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
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
set_path_for_multishellhelper wrapsstd::env::set_varin anunsafeblock even though the call is safe; you can drop theunsafeblock entirely to avoid suggesting undefined behavior risk where there is none. - In
Use::apply, routing informational messages to stderr by switching the log level fromInfotoErrorwheninfo_to_stderris true may confuse downstream consumers that interpret log levels; consider a dedicated macro/flag for stderr output that preserves the semantic log level.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `set_path_for_multishell` helper wraps `std::env::set_var` in an `unsafe` block even though the call is safe; you can drop the `unsafe` block entirely to avoid suggesting undefined behavior risk where there is none.
- In `Use::apply`, routing informational messages to stderr by switching the log level from `Info` to `Error` when `info_to_stderr` is true may confuse downstream consumers that interpret log levels; consider a dedicated macro/flag for stderr output that preserves the semantic log level.
## Individual Comments
### Comment 1
<location path="src/commands/env.rs" line_range="62-77" />
<code_context>
}
}
+fn set_path_for_multishell(multishell_path: &std::path::Path) {
+ let path_for_node = if cfg!(windows) {
+ multishell_path.to_path_buf()
+ } else {
+ multishell_path.join("bin")
+ };
+
+ let current_path = std::env::var_os("PATH").unwrap_or_default();
+ let mut split_paths: Vec<_> = std::env::split_paths(¤t_path).collect();
+ split_paths.insert(0, path_for_node);
+ if let Ok(new_path) = std::env::join_paths(split_paths) {
+ unsafe {
+ std::env::set_var("PATH", new_path);
+ }
</code_context>
<issue_to_address>
**suggestion:** Remove the unnecessary `unsafe` block around `std::env::set_var`.
`std::env::set_var` is safe and does not require an `unsafe` block. Restricting `unsafe` to truly unsafe operations makes it easier to identify real safety boundaries and avoids suggesting extra invariants are needed here.
```suggestion
fn set_path_for_multishell(multishell_path: &std::path::Path) {
let path_for_node = if cfg!(windows) {
multishell_path.to_path_buf()
} else {
multishell_path.join("bin")
};
let current_path = std::env::var_os("PATH").unwrap_or_default();
let mut split_paths: Vec<_> = std::env::split_paths(¤t_path).collect();
split_paths.insert(0, path_for_node);
if let Ok(new_path) = std::env::join_paths(split_paths) {
std::env::set_var("PATH", new_path);
}
}
```
</issue_to_address>
### Comment 2
<location path="CHANGELOG.md" line_range="23" />
<code_context>
+
+### Patch Changes
+
+- [#1268](https://github.com/Schniz/fnm/pull/1268) [`ddab191`](https://github.com/Schniz/fnm/commit/ddab1911dc33a24aeec9d2b8139df1916c518d64) Thanks [@isaacl](https://github.com/isaacl)! - Make `fnm default` with no trailing positional argument to return the default version
+
+- [#1517](https://github.com/Schniz/fnm/pull/1517) [`d3747af`](https://github.com/Schniz/fnm/commit/d3747affd604223c752171c14944bd5773096b09) Thanks [@Schniz](https://github.com/Schniz)! - Improve `--use-on-cd` shell hook robustness by de-duplicating the zsh hook on re-source and fixing Windows CMD hook behavior for paths with spaces and drive changes.
</code_context>
<issue_to_address>
**suggestion (typo):** Rephrase this sentence to fix slightly awkward grammar.
The phrase "Make `fnm default` with no trailing positional argument to return the default version" is ungrammatical. Please rephrase to something like "Make `fnm default` with no trailing positional argument return the default version."
```suggestion
- [#1268](https://github.com/Schniz/fnm/pull/1268) [`ddab191`](https://github.com/Schniz/fnm/commit/ddab1911dc33a24aeec9d2b8139df1916c518d64) Thanks [@isaacl](https://github.com/isaacl)! - Make `fnm default` with no trailing positional argument return the default version
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Signed-off-by: Dargon789 <64915515+Dargon789@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request bundles a series of features, bug fixes, and enhancements for the fnm 1.39.1 release. The changes are comprehensive, including improved use-on-cd behavior for better performance and robustness, expanded architecture support, and documentation updates. The code quality is high, with new functionality well-tested and integrated. I have one suggestion to improve code safety by removing an unsafe block. Overall, this is an excellent set of changes preparing for the new release.
Owner
Author
|
@Mergifyio refresh |
✅ Pull request refreshed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Release fnm 1.39.1 with improved env/use-on-cd behavior, expanded architecture support, and more robust installer and shell integration.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests:
Chores: