pkgs/profpatsch/nman: ✨ add --file flag to use local nix file#59
pkgs/profpatsch/nman: ✨ add --file flag to use local nix file#59Profpatsch wants to merge 8 commits intomasterfrom
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
a192a6f to
ffb8977
Compare
sternenseemann
left a comment
There was a problem hiding this comment.
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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.
| Use the specified .nix file | |
| Use the Nix expression at |
There was a problem hiding this comment.
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.
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.
This is a bit icky.
There was a problem hiding this comment.
Not sure how to do it better tbh
There was a problem hiding this comment.
Does while let Some(arg) = … entitle us to call next() inside the loop? Surely something like that must be possible.
| } | ||
|
|
||
| // 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.
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.
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 <noreply@anthropic.com>
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.
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.
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.
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.
but this matches if we have set an error already!!! you've made it worse!!!!
There was a problem hiding this comment.
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.
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