ryan4yin / nixos-rk3588

Minimal NixOS running on RK3588/RK3588s based SBC(Orange Pi 5 Plus, Orange Pi 5, Rock 5A, etc)
MIT License
152 stars 28 forks source link

Better defaults #14

Closed i1i1 closed 11 months ago

i1i1 commented 1 year ago

This pr is a messy a bit, because it does everything. Better look at individual commits.

Overall, it started off with removing unnecessary defaults in the first commit and better export of nixos modules in the second one.

Latter commits are misc, they just do formatting of nix files and add pre commit hooks (with default nix develop environment) with linting (you can also lint and check formatting with nix flake check now).

I can split the first 2 commits and latter ones in separate pull request if you want, but I just found it easier for now.

alex-morgun commented 11 months ago

Hard to tell what actually changed and what improvements are. Why are these defaults are better?

i1i1 commented 11 months ago

Hard to tell what actually changed and what improvements are.

Take a look at each individual commit.

Why are these defaults are better?

Fair enough! I've

alex-morgun commented 11 months ago

I have an idea how to do the improvements easy to review way

  1. Make a PR with this alone

Latter commits are misc, they just do formatting of nix files and add pre commit hooks (with default nix develop environment) with linting (you can also lint and check formatting with nix flake check now).

  1. Another PR with mkDefault
  2. Another for user/group, but personally I like the original variant
i1i1 commented 11 months ago

I have an idea how to do the improvements easy to review way

I think it is quite easy to follow this pr commit by commit. Most of the changes are minor anyway. This pr is mostly yak shaving.

ryan4yin commented 11 months ago

pre-commit-hook is a good thing, could you please submit the two changes of pre-commit-hook addition and code formatting / linting respectively? In addition, I personally prefer alejandra to nixpkgs-fmt, could you replace this part?

ryan4yin commented 11 months ago

And:

  1. Another PR for Export nixos modules & flake-utils.
  2. Another PR with some minor changes(mkDefault/Sanitize special argument)
  3. As for user/group, I agreed with @alex-morgun , just keep it unmodified.

Thanks for the PR!

i1i1 commented 11 months ago

I'll split this pr into several ones. I just have a couple of questions.

Another PR with some minor changes(mkDefault/Sanitize special argument)

Sanitizing special argument does feel related to exporting nixos modules. Btw, maybe you know some way to not require users to pass special arguments at all?

As for user/group ... just keep it unmodified

It just feels weird to name it like that, even though it is more similar to the default convention of having configuration.nix

Also, am I getting it right that you want to have 3 prs as follows:

ryan4yin commented 11 months ago

Sanitizing special argument does feel related to exporting nixos modules. Btw, maybe you know some way to not require users to pass special arguments at all?

I'm a little confused with this commit, I'll take a look at this one after other PRs getting merged.

Also, am I getting it right that you want to have 3 prs as follows:

* Export nixos modules & flake-utils

* `pre-commit-hook` and code formatting

* Other minor changes

Yep, I hope to merge these PR one by one, and then see what else needs to be done(NixOS 23.11, latest Linux mainline kernel, etc...).

i1i1 commented 11 months ago

Closing in favor of #15 #16 #17