docs: design den dendritic migration#46
Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Code Review
This pull request migrates the repository to a Den dendritic layout, restructuring configuration into modular aspects, hosts, and users while implementing flake-file for automated flake.nix generation. The review identifies several critical logic errors, specifically the omission of the den argument in function headers across multiple modules, which will prevent successful evaluation. Furthermore, the home-manager darwin module is not correctly imported, a shell command in the aerospace configuration is left incomplete, and user identity metadata is hardcoded in the Git aspect instead of being centralized for maintainability.
| {inputs, ...}: let | ||
| hostConfig = { | ||
| darwin.home-manager = { | ||
| useUserPackages = true; | ||
| useGlobalPkgs = true; | ||
| backupFileExtension = "backup"; | ||
| sharedModules = [ | ||
| inputs.catppuccin.homeModules.catppuccin | ||
| inputs.stylix.homeModules.stylix | ||
| inputs.lazyvim.homeManagerModules.lazyvim | ||
| inputs.nvf.homeManagerModules.default | ||
| inputs.nix4nvchad.homeManagerModules.default | ||
| inputs.spicetify-nix.homeManagerModules.spicetify | ||
| ]; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
This module has two critical issues:
- The
denargument is missing from the function header, which will cause an 'undefined variable' error when accessingden.aspectson line 47. - The
home-managermodule fornix-darwinis not imported. Withoutinputs.home-manager.darwinModules.home-managerin theimportslist, thedarwin.home-manageroptions will not be defined, leading to evaluation errors.
References
- The migration plan explicitly required importing the home-manager darwin module and using the den argument. (link)
- Nix modules must import the necessary provider modules (like home-manager's darwin module) to make their options available.
| _: let | ||
| mod = "alt"; | ||
| aerospace = { | ||
| homeManager = {lib, ...}: { | ||
| services.jankyborders = { | ||
| enable = true; | ||
| settings = { | ||
| active_color = "0xffcba6f7"; | ||
| hidpi = "on"; | ||
| }; | ||
| }; | ||
| programs.aerospace = { | ||
| enable = true; | ||
| launchd.enable = true; | ||
| settings = { | ||
| key-mapping = { | ||
| preset = "colemak"; | ||
| key-notation-to-key-code = { | ||
| g = "b"; | ||
| d = "g"; | ||
| h = "m"; | ||
| z = "x"; | ||
| x = "c"; | ||
| c = "d"; | ||
| b = "z"; | ||
| m = "h"; | ||
| }; | ||
| }; | ||
| default-root-container-layout = "tiles"; | ||
| automatically-unhide-macos-hidden-apps = true; | ||
| exec-on-workspace-change = [ | ||
| "/bin/bash" | ||
| "-c" | ||
| ]; |
There was a problem hiding this comment.
This module contains critical errors:
- The
denargument is missing from the header, causing an error on line 147. - The
exec-on-workspace-changecommand is incomplete.["/bin/bash", "-c"]requires a command string as the third element. If no script is intended, this block should be removed to avoid execution errors.
| @@ -0,0 +1,22 @@ | |||
| # Common nix-darwin host settings. | |||
| _: let | |||
| @@ -0,0 +1,8 @@ | |||
| # Default user context. | |||
| {lib, ...}: { | |||
| _: let | ||
| git = { | ||
| homeManager = { | ||
| programs.git = { | ||
| enable = true; | ||
| settings = { | ||
| user = { | ||
| name = "phucisstupid"; | ||
| email = "125681538+phucisstupid@users.noreply.github.com"; | ||
| }; |
There was a problem hiding this comment.
- The
denargument is missing from the header, which will break the build on line 46. - User identity (name/email) is hardcoded. Since this migration aims for a dendritic layout with shared aspects, consider using a central profile aspect for user metadata to ensure consistency across Git, Jujutsu, and other tools.
No description provided.