Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flake: adjustments for extensibility, usability, & maintenance #133

Merged

Conversation

spikespaz
Copy link
Member

@spikespaz spikespaz commented Aug 27, 2024

  • I like your style of { self, ... }@inputs: with inputs; but I find that for this flake, there are a small number of inputs and all of them are used in the top-level. Therefore, I've named them all as proper arguments.
  • First thing I did, to keep clean patches with minimal whitespace changes, is select nixfmt-classic as the Flake's formatter. I find it to be the only formatter that minimizes the effects of whitespace and indentation changes on VCS history.
  • Remove flake-utils. You don't need to add a dependency just to do something like flake-utils.lib.eachDefaultSystem, because lib.genAttrs and lib.mapAttrs accomplish the exact same thing, and don't hide information from newcomers.
  • Introduce nix-systems to allow the Flake outputs to be generic over a systems list. See their README for more information, I strongly stand by this pattern. If you would like, we can replace the systems input with a single file.
  • Alias packages.${system}.nix4vscode as packages.${system}.default.
  • Add a Nixpkgs overlay for the nix4vscode package. This overlay will use the pinned version of Rust from rust-toolchain.toml through rust-overlay's rust-bin interface, always.
  • You may want to drop this one if you see it as not necessary, but the development shell now includes tools from Rust nightly which require nightly rustc flags to run with full features (cargo-expand, doc, RUST_BACKTRACE, etc.).
  • Set strictDeps = true where appropriate.
  • The flake's package(s) are now inherited from overlay application, meaning that our nixpkgs import and properly utilized.

I recommend the commits are reviewed individually, this way, if you disagree with my some elements of my style we can rebase appropriately. I am expecting that I will either have to debate removal of flake-utils or walk it back completely, but I stand by that we should not add an input for a single function, especially when it interferes with the composability afforded by nix-systems.

@spikespaz spikespaz requested a review from cathaysia August 27, 2024 03:42
@spikespaz spikespaz force-pushed the opinionated-nix-flake branch 2 times, most recently from a9d611d to 8efbe7b Compare August 27, 2024 04:00
@spikespaz spikespaz force-pushed the opinionated-nix-flake branch from 8efbe7b to 522c9f2 Compare August 27, 2024 04:07
@spikespaz spikespaz force-pushed the opinionated-nix-flake branch from 522c9f2 to 716ee76 Compare August 27, 2024 04:08
@cathaysia cathaysia enabled auto-merge (rebase) August 27, 2024 05:32
@cathaysia cathaysia merged commit ccdd90e into nix-community:master Aug 27, 2024
3 checks passed
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