Skip to content

Conversation

Noah765
Copy link

@Noah765 Noah765 commented Jul 13, 2024

Thank you for this awesome project. I have created a fork of it and worked a bit on improving it. Here are the most important changes I made:

  • Top-level inputs, osImports and hmImports: Importing modules and adding inputs shouldn't have to be separated from the modules imports and hidden in the config section of it. The osModules and hmModules options are still functional.
  • Better error messages: I think I have improved the error messages all around, especially for defining flake inputs.
  • The useHomeManager config attribute can be used to toggle Home Manager per config.
  • The useHm module arg is useful for conditional logic around enabling Home Manager
  • No initialInputs are needed anymore, nixpkgs and home-manager can be added elsewhere.
  • osOptions and hmOptions: These extend the normal options arg to make copying options easier.

Breaking changes:

  • You have to define your stateVersion either as an attribute in mkFlake, or as an attribute per config.
  • The combinedManager module arg now gets you the evaluated root of Combined Manager, rather than it's path. The path is provided as combinedManagerPath, if you need it.
  • Removed the combinedManager.osPassedArgs and combinedManager.osExtraPassedArgs options. Arguments to NixOS modules can be added via specialArgs (which is passed to Combined Manager, as well as NixOS and Home Manager modules), or via os._module.args.

Copy link
Owner

@FlafyDev FlafyDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for the PR! It looks awesome!
I'll take a more in-depth look at it soon.

for now:

  1. Not sure about this:

Top-level inputs, osImports and hmImports: Importing modules and adding inputs shouldn't have to be separated from the modules imports and hidden in the config section of it. The osModules and hmModules options are still functional.

I've made inputs not part of the modules' config because I didn't want people to try to use module functions on it(like mkIf) and get weird behaviors. Putting it outside of config shows that it's like imports, not something that can be changed conditionally.

I made osModules and hmModules as options and not on the same level as imports and inputs because they can be changed conditionally most of the time.

  1. The PR is really hard to read.
  • Commits are not documented
  • The first commit is formatting of the entire project.. That means it's really hard for me to see what the PR changes (same thing for restructuring). Also, please use the project's formatter: alejandra

do you think you could consider these points and organize the commits so it'd be easier for me to review?

  1. why remove the nix-patches folder? it allows for versioned patches
  2. what does that mean? "Note that the options being copied are complex, so you wouldn't want to just copy and paste them into your option declarations." What does it mean complex options?

@Noah765
Copy link
Author

Noah765 commented Jul 13, 2024

Hi, here are some comments on the points you made:

  1. I think osModules and hmModules (rebranded as osImports and hmImports) are semantically very similar to module imports. You could sometimes add them conditionally using nix syntax, but you shouldn't have to, because that's what options are for. I thought it was rather odd to treat these imports differently from module imports.
    In my fork, mkIfs, mkMerges and mkOverrides are basically ignored for inputs, osImports, osModules, hmImports and hmModules, meaning that inputs = mkIf false { someInput.url = "someUrl"; }; adds the input, the branch condition isn't even evaluated. This makes it possible to define inside config = mkIf option.enable {}; blocks. osImports, osModules, hmImports and hmModules are evaluated in the same way (also for technical reasons, I don't think I could get them to work with osOptions or something like that).
  2. I'm sorry for not using the correct formatter, I think I just had a hard time reading your original code, and formatting it with my preferred formatter helped (I didn't even realise there was a formatter defined for this project at the time).
    I changed so much during development that it might be easier to just compare the result of this PR with your original code.
    In the interest of full transparency, I'm very new to contributing to open source, and am currently participating in a challenge that requires frequent commits, but I still could have structured my changes a bit better.
    I think it's going to be very difficult to organise my commits now, because they often don't just change one thing, but are more like time stamps of my work, which sometimes goes all over the place.
  3. I don't know why I removed it, I'll put it back in, and also preemptively add a patch for the latest version of Nix, which is not yet on unstable.
  4. If you take a look at the option I copied, you'll see that the type definition is very complex, and I wouldn't want to copy and paste the whole thing. The options are defined here, but the type definition includes all of this code.

@Noah765
Copy link
Author

Noah765 commented Jul 14, 2024

I spent a few hours trying to organise the changes and created a new PR: #7, so I am closing this one in favour of the new PR.

@Noah765 Noah765 closed this Jul 14, 2024
@Noah765 Noah765 deleted the main branch July 15, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants