-
Notifications
You must be signed in to change notification settings - Fork 2k
Improve error message when plugin add fails because config directory doesn't exist (Issue #10754)
#16999
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
Improve error message when plugin add fails because config directory doesn't exist (Issue #10754)
#16999
Conversation
Fixes nushell#10754 When registering a plugin fails because the config directory doesn't exist or isn't writable, the error message was unhelpful: "Plugin failed to load: No such file or directory (os error 2)" This change: - Checks if the parent directory exists before creating the plugin registry file - Automatically creates the directory if it's missing (like other Nu commands) - Provides clear error messages using ErrorKind::DirectoryNotFound - Shows the actual directory path that's problematic with helpful context Now users see actionable errors that distinguish between: - Plugin binary file not found - Config directory doesn't exist or can't be created 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Apply cargo fmt formatting to multi-line nested imports.
- Collapse nested if statements using let-chain syntax - Replace useless format!() with .to_string()
plugin add fails because config directory doesn't exist (Issue #10754)
|
Hi! This is my first time contributing. I did my best to follow the contribution guidelines. Since this is such a small bug fix, I'm not sure if this change warrants updating documentation, but I left the task in the PR description for now. Let me know if you'd like me to update it! |
WindSoilder
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thank you for your contribution, the code looks good to me.
Can you add a test for the behavior? You can add it to tests/plugins/registry_file.rs.
…it doesn't exist when running plugin add
Done! Thanks for the feedback. |
WindSoilder
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
|
The CI failed, can you run |
Done. Sorry about that! |
cptpiepmatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I have some nitpicks. You don't have to do them if you don't want to or think they are good as they are.
crates/nu-cmd-plugin/src/util.rs
Outdated
| // Save the modified file on success | ||
| // First, ensure the parent directory exists | ||
| if let Some(parent_dir) = plugin_registry_file_path.parent() | ||
| && !parent_dir.exists() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nitpick but you don't have to do this check, just run the fs::create_dir_all, it already checks if the necessary dirs exist.
crates/nu-cmd-plugin/src/util.rs
Outdated
| file_span, | ||
| parent_dir.to_path_buf(), | ||
| "Cannot create plugin config directory. \ | ||
| Ensure the parent path is accessible and you have write permissions" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you have to write this long context. If you don't have the permissions to create such a dir, you already get a permissions denied error, making this very obvious. Very long messages in our messages get somewhat messy. You don't have to remove that, just wanna say. You can take a look how it looks in your terminal.
|
Can you add a before and after to the changelog. That would make it easier to see what you did when reading the changelog. You can do it similar to this: #16892 |
Thanks for the feedback! I appreciate your suggestions. I don't want to add anything unnecessary, so I'm happy to clean that up. I'll also work on adding a clear before / after change log. |
|
@cptpiepmatz I applied changes in response to your feedback and updated the PR description with a before / after changelog. Please let me know if there's anything else you think could be improved! |
|
Thank you for your changes but this is not really what I asked for, sorry. The changelog is mainly for nushell users not developers. So I expected a terminal output, what you would see as a nushell user. |
Oh got it. Sorry about that, I misunderstood. I'll update that. |
|
updated the changelog! I wasn't 100% sure about including the let me know if you have any feedback @cptpiepmatz |
cptpiepmatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I like that. Let's wait for the pipeline and then we can merge. ❤️
Fixes #10754
This PR improves the user experience when running
plugin addby automatically creating missing plugin config directories and providing clear, actionable error messages.Changes
When running nushell from a terminal with a configuration file path to a location where no file exists:
Before
Running this command throws an unhelpful error:
After
Running this command will succeed because the configuration file will be created automatically if it doesn't exist:
Release Notes
Improved error messages when plugin config directory doesn't exist during
plugin add. The command now automatically creates missing parent directories, aligning with behavior in other Nushell commands.Tasks after submitting