WIP: Merging aspects #18
Conversation
There was a problem hiding this comment.
Pull request overview
This WIP PR refactors the type system and test infrastructure for the flake-aspects project. The changes primarily consolidate type definitions for better reusability and split a monolithic test file into individual, more maintainable test modules.
Key changes:
- Simplified type definitions by reusing
aspectsTypefor theprovidesoption, eliminating duplication - Refactored the order of type definitions, placing
providerTypeto acceptaspectSubmoduleAttrsfirst, thenfunctionProviderType - Split
checkmate/modules/tests.nixinto 14 individual test files for better organization and maintainability
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| nix/types.nix | Refactored type definitions: simplified aspectsType usage for freeformType, reordered providerType definition, and replaced inline submodule with aspectsType for provides option |
| checkmate/modules/tests.nix | Completely restructured from a monolithic test file to a minimal module args provider that exports helper functions and imports |
| checkmate/modules/tests/*.nix | New individual test files covering various aspect functionality: dependencies, provides, fixpoints, parametric aspects, default providers, chain resolution, and transpose operations |
| checkmate/modules/checkmate.nix | New configuration file for checkmate module importing nix-unit and treefmt-nix with formatter settings |
| checkmate/flake.nix | New flake configuration for the checkmate test infrastructure with proper input dependencies |
| checkmate/flake.lock | Lock file for checkmate flake dependencies |
| .github/workflows/test.yml | Updated workflow to reference the checkmate subdirectory using ?dir=checkmate parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Ok, now I'm thinking to re-work the aspects definition, I think the problem is we have a custom type of function-to-aspect. Aspects are modules anyways and modules in nix are "attrset or function-to-attrset", maybe we could explore removing our function-to-aspect, and just make the "context" available as part of the aspect-module |
|
Hm, but the problem araises from deeply nested modules, via |
When merging aspect configs across scopes (e.g. bar = config.foo), the modules and resolve options were incorrectly merging pre-computed values from individual scopes instead of recomputing from the merged config. The fix uses custom type merge functions that always recompute from the final merged config, ensuring resolved modules reflect all merged aspect definitions.
* Revert "Split tests (#19)" This reverts commit 72ffc00. * update README with correct test commands (#22) * Revert "Revert "Split tests (#19)"" This reverts commit 1df56b4. * test merging aspects * fix: use custom merge functions for modules and resolve options (#23) When merging aspect configs across scopes (e.g. bar = config.foo), the modules and resolve options were incorrectly merging pre-computed values from individual scopes instead of recomputing from the merged config. The fix uses custom type merge functions that always recompute from the final merged config, ensuring resolved modules reflect all merged aspect definitions. --------- Co-authored-by: Albert O'Shea <albertoshea2@gmail.com>
This PR is for solving vic/den#113
superseded by #25