Beastwick18 / nyaa

A tui tool for browsing and downloading torrents
https://crates.io/crates/nyaa
GNU General Public License v3.0
407 stars 7 forks source link

nix(flake): avoid importing nixpkgs #25

Closed diniamo closed 4 months ago

diniamo commented 4 months ago

https://zimbatm.com/notes/1000-instances-of-nixpkgs

I'm sorry, but the rest of the flake is also a horror to look at.

Beastwick18 commented 4 months ago

Changes LGTM, I'll go ahead and merge. Also, in what way is it a horror? I'm new to Nix and this is my first time creating a flake so suggestions would be appreciated.

diniamo commented 4 months ago
  1. Why are tabs used for indentation? I recommend alejandra for formatting
  2. Don't use flake-utils, very ugly, and abstractions like this are generally bad. The only acceptable one is flake-parts
  3. I personally use the buildRustPackage function from nixpkgs, never really seen a reason to use crane or naersk, but if there is one, then enlighten me
  4. Declaring every option from the config file is a bad idea for countless reasons (the main ones are that it makes it impossible to maintain, and in your case, you had to do that funny stuff with making every option nullable, and filtering null options - all of which can be avoided), if you take a look at some home manager modules, they just serialize an attribute set directly to the config file
Beastwick18 commented 4 months ago

Why are tabs used for indentation? I recommend alejandra for formatting

No real reason. I should be using a formatter though so I'll try that one out.

Don't use flake-utils, very ugly, and abstractions like this are generally bad. The only acceptable one is flake-parts

I don't think I'm using flake-utils, unless its an indirect dependency from naersk?

I personally use the buildRustPackage function from nixpkgs, never really seen a reason to use crane or naersk, but if there is one, then enlighten me

From what I understand, naersk will improve build times for Rust projects by reading the Cargo.toml and caching dependencies. Basically installing each dependency as its own package in the nix store so they don't have to be rebuilt. So when updating the package only new dependencies and the resulting binary have to be built.

This may be undesired, though. I just thought it would be a good idea to improve build times.

Declaring every option from the config file is a bad idea for countless reasons (the main ones are that it makes it impossible to maintain, and in your case, you had to do that funny stuff with making every option nullable, and filtering null options - all of which can be avoided), if you take a look at some home manager modules, they just serialize an attribute set directly to the config file

I did that so there would be explicit types associated with each option. So if the config expects a list of strings then that can be type-checked when building the resulting config. From what I understand this is not that uncommon. For example, the ssh module for home-manager lists out all options with types, filters them, and then writes them to disk. It does have an option for extraConfig, which isn't checked at all, but this config is well-defined so that's not necessary here.

The reason I have to filter out null values is because TOML does not support it, so it will throw an error while building if any values are null. The only options that should be nullable are the ones which are not required, anyways.

It's also not a big deal for me to manually add each option, since I don't frequently add many new config options. The bulk of the work has already been done and only 1-2 new options are typically added each update.

diniamo commented 4 months ago

I don't think I'm using flake-utils, unless its an indirect dependency from naersk?

Oops my bad, what you are doing is fine there.

From what I understand, naersk will improve build times for Rust projects by [...] caching dependencies.

Hm, that's fine I guess, never really cared about that.

[...]

You are greatly missing the point here, but have it your way, doesn't really matter from the user's perspective.