-
Notifications
You must be signed in to change notification settings - Fork 11
pkgs/profpatsch/nman: ✨ add --file flag to use local nix file #59
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
base: master
Are you sure you want to change the base?
Conversation
Add support for using ./default.nix instead of <nixpkgs> when the --local or -l flag is specified. This allows viewing man pages for packages defined in local development environments or custom package sets. Changes: - Add use_local field to Main struct - Parse --local/-l command line option - Conditionally use ./default.nix in Nix expression generation - Update help text and man page documentation with new option and examples - Maintain backward compatibility (default behavior unchanged) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
When using dotted attribute paths like `pkgs.profpatsch.nman`, nman was incorrectly trying to find a man page named "pkgs.profpatsch.nman" instead of extracting the actual page name "nman" from the last component. Changes: - Add extract_page_name_from_attr() helper that splits on '.' and returns last component - Update CLI parsing to use helper for auto-detected page names (cases 2 and 3 args) - Add comprehensive test coverage for the helper function - Maintain backward compatibility for simple attribute names and explicit page names - Add description to the man page Now `nman --local pkgs.profpatsch.nman` correctly builds the full attribute but searches for the "nman" man page instead of "pkgs.profpatsch.nman". 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
a192a6f to
ffb8977
Compare
sternenseemann
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.
I think the specific --local flag is a bit silly. I'd prefer just adding -f/--file like the new Nix/Lix CLI does. Then you'd write nman -f . mypkg.
|
I special-cased it for |
Refactor the inflexible --local flag to use -f/--file <path> syntax,
matching modern Nix CLI conventions. This allows specifying any .nix
file instead of hardcoding ./default.nix.
Usage: nman -f . mypkg (equivalent to old --local behavior)
nman -f /path/to/file.nix mypkg (new flexibility)
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
Unify the logic for determining the import path and generate the Nix expression only once, reducing code duplication and making the logic clearer. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
160d708 to
76c7c94
Compare
| .Nm | ||
| into verbose mode, where it prints all commands it executes. | ||
| .It Fl -file Ar path | f Ar path | ||
| Use the specified .nix file |
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.
| Use the specified .nix file | |
| Use the Nix expression at |
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.
No it only allows nix files? I think. We pass it to import but if we want to allow arbitrary expressions we’d have to escape them properly first
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.
It is clear that it's a file from what follows.
| let mut positional_args = Vec::new(); | ||
|
|
||
| let mut i = 1; // Skip program name | ||
| while i < all_args.len() { |
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 bit icky.
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.
Not sure how to do it better tbh
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.
Does while let Some(arg) = … entitle us to call next() inside the loop? Surely something like that must be possible.
pkgs/profpatsch/nman/nman.rs
Outdated
| } | ||
|
|
||
| // Only process positional arguments if we haven't already set an error | ||
| if let ShowUsage { err_msg: Some("Unexpected number of arguments") } = cli_res { |
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.
We are matching an error message to detect that no error occurred? This is extremely brittle and also super confusing code.
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.
Yeah it should not match on the exact error, any error should skip
Newer rust versions apparently can specify the extern directly without the link annotation (was a warning).
This should pass the right kinds of path to nix in most situations.
bcd41b6 to
0aafc7f
Compare
Replace tuple-style CliAction::Man with inline struct fields for better readability and maintainability. Move file_path from Main struct to the action itself for better separation of concerns. Changes: - Use named fields (attr, section, page, file_path) instead of positional args - Keep efficient &'a str lifetimes for all string references - Pass file_path explicitly to open_man_page method - Remove file_path from Main struct 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
When we match positional args, we want to skip whenever we are already in the ShowUsage case.
4539d97 to
f7c85c5
Compare
|
|
||
| #[link(name = "c")] | ||
| extern { | ||
| extern "C" { |
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.
Wouldn't it be better to be explicit?
| extern "C" { | |
| #[link(name = "c")] | |
| extern "C" { |
| // like <vuizvui>, /home/lukas/src/nix/nixpkgs, … | ||
| let nix_import = match file_path { | ||
| Some(path) => { | ||
| if path.starts_with('/') { |
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.
Should also check if the path matches ~/… or <…> which are also valid Nix paths.
I guess we should also clarify in the man page that we allow this special path syntax.
Could be nice to add a check whether it is unnecessary to add the ./ prefix, but it also doesn't really matter.
| attr: &'a str, | ||
| section: Option<&'a str>, | ||
| page: &'a str, | ||
| file_path: Option<&'a str>, |
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.
Maybe we should clarify that we treat this as a subset of Nix syntax which is passed on, hence we don't use Path.
| } | ||
|
|
||
| // Only process positional arguments if we haven't already set an error | ||
| if let ShowUsage { err_msg: _ } = cli_res { |
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.
but this matches if we have set an error already!!! you've made it worse!!!!
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.
You could also return a Result from either main or a dedicated function and return early instead of adding your own Resultish type. For example if you want to have different errors with different actions, you could still use Result<_, CustomErrorEnum>.
|
|
||
| // Only process positional arguments if we haven't already set an error | ||
| if let ShowUsage { err_msg: _ } = cli_res { | ||
| cli_res = match positional_args.len() { |
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.
Can't you match on a slice of positonal_args?
Add support for using ./default.nix instead of when the --local or -l flag is specified. This allows viewing man pages for packages defined in local development environments or custom package sets.
Changes:
🤖 Generated with Claude Code