test accessing namespaces from inputs #114
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for namespace functionality, specifically testing the ability to create local namespaces, merge multiple namespace sources from inputs, and access namespaces via various methods (direct access, angle brackets, and flake outputs). The changes refactor the namespace function to accept an array of sources instead of a single input, enabling merging of namespaces from multiple flakes.
Key changes:
- Refactored
namespace()to accept array of sources (flake inputs) with optional boolean flag for flake output exposure - Fixed bug in
den-brackets.nixpreventing errors when accessing single-element namespace paths - Added comprehensive test demonstrating local namespace creation, multi-source namespace merging, and various access patterns
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| nix/namespace.nix | Refactored to accept array of sources, enabling multi-source namespace merging; changed flake output type to lib.types.raw |
| nix/den-brackets.nix | Fixed empty list bug when accessing namespace root by adding length check before accessing tail |
| templates/default/modules/namespace.nix | Updated comments and examples to reflect new array-based API; added flake.foo assignment |
| templates/examples/modules/_example/ci/namespace.nix | Added comprehensive tests for namespace creation, merging, and access patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nix/namespace.nix
Outdated
| name: input: | ||
| { config, lib, ... }: | ||
| name: sources: | ||
| { config, lib, options, ... }: |
There was a problem hiding this comment.
[nitpick] The options parameter is declared but never used in this module. Consider removing it from the function parameters unless it's needed for a specific purpose.
| imports = [ (inputs.den.namespace "eg" false) ]; | ||
| # create an `eg` (example!) namespace. (flake exposed) | ||
| imports = [ (inputs.den.namespace "eg" true) ]; | ||
| flake.foo = den; |
There was a problem hiding this comment.
This line appears to be unrelated to namespace configuration and looks like leftover test/debug code. Consider removing flake.foo = den; from the default template as it doesn't demonstrate any namespace functionality and may confuse users.
| flake.foo = den; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| denfuls = map (lib.getAttrFromPath [ | ||
| "denful" | ||
| name | ||
| ]) (builtins.filter builtins.isAttrs from); |
There was a problem hiding this comment.
The use of lib.getAttrFromPath here will throw an error if any input in the sources array doesn't have the denful.${name} attribute path. Consider using lib.attrByPath with a default value (e.g., {}) or adding validation to provide a clearer error message when a source is missing the expected denful namespace.
Example:
denfuls = map (input:
lib.attrByPath ["denful" name] {} input
) (builtins.filter builtins.isAttrs from);| denfuls = map (lib.getAttrFromPath [ | |
| "denful" | |
| name | |
| ]) (builtins.filter builtins.isAttrs from); | |
| denfuls = map (input: | |
| lib.attrByPath [ "denful" name ] {} input | |
| ) (builtins.filter builtins.isAttrs from); |
| (inputs.den.namespace "eg" true) | ||
|
|
||
| # you can also mount a namespace from many input sources. | ||
| # the second argument becomes an array of inputs. |
There was a problem hiding this comment.
[nitpick] The wording "becomes an array" is slightly imprecise. Consider rewording to "the second argument is an array" or "pass an array as the second argument" for clarity, since the argument doesn't transform but is provided as an array.
| # the second argument becomes an array of inputs. | |
| # the second argument is an array of inputs. |
| # you can also import namespaces from remote flakes. | ||
| # imports = [ (inputs.den.namespace "ours" inputs.theirs) ]; | ||
| # you can also merge many namespaces from remote flakes. | ||
| # keep in mind a namespace is defined only once, so give it an array: |
There was a problem hiding this comment.
[nitpick] The phrase "a namespace is defined only once" could be clearer. Consider rewording to something like "define each namespace only once and pass all sources together in an array" to make it explicit that you shouldn't call inputs.den.namespace "name" multiple times with different sources.
| # keep in mind a namespace is defined only once, so give it an array: | |
| # define each namespace only once and pass all sources together in an array: |
Fix #113