Misterio77 / nix-colors

Modules and schemes to make theming with Nix awesome.
GNU General Public License v3.0
465 stars 38 forks source link

Added conversion functions, helper functions, and tests. #18

Closed djacu closed 1 year ago

djacu commented 2 years ago

Adds functions for converting from hex strings to RGB values. Addresses comments in #15.

Misterio77 commented 2 years ago

Great work, thank you!

May I suggest that maybe we could try removing the reliance on nixpkgs? Similarly to what flake-utils do. I think if we can get away with it, it would be really awesome!

I understand that we're using just lib (not lib-pkgs), which is pure nix. So we don't have to actually instantiate (by using system prefixes) the part of nixpkgs lib that relies on external derivations (which is something I want to avoid outside of contrib).

But my reasoning is that it adds yet another nixpkgs input (forces them to use inputs.follows to avoid it) for the user, slows down fresh evaluation (nixpkgs takes a while to fetch).

We basically just need to vendor the nixpkgs functions in (we can perhaps make a directory for it and organize the functions I'm currently vendoring together with them), this is what flake-utils does.

Please let me know what you think!

If you agree with it but are short on time to make the changes, I can handle them, if that would be okay with you.

Misterio77 commented 2 years ago

A little wishlist for follow up PR's we might try after this one is merged (for anyone reading, help is very welcome!):

djacu commented 2 years ago

Thank you! Glad I could contribute back. :)

Yeah I could certainly remove the dependency on nixpkgs. I think the functions I am using should be easy to implement. Actually, I asked around and someone pointed me to this repo in nix-community. What do you think about using this? Seems like a lightweight flake that gives us all the capabilities of nixpkgs.lib without all the overhead of nixpkgs. It would also give us the debug module for testing. Thoughts?

Actually, there are some things I should fix regardless of the path we take. I can't seem to change the PR to a draft. Or I could just put [WIP] in the title until it is ready for review.

djacu commented 2 years ago
* Testing the yaml conversion functions (and everything else, really)

That was actually another one of the things I wanted to fix and improve on. I have some tests in there but they import things in a bad way and they aren't tied into the top level flake so we can use things like nix flake check. Is this what you were planning?

Misterio77 commented 2 years ago

Actually, I asked around and someone pointed me to this repo in nix-community. What do you think about using this?

This is awesome! Exactly what we need, great find. Love this community :)). For sure we should use it for both our lib and testing infraestructure.

Or I could just put [WIP] in the title until it is ready for review.

I think it's okay. I don't have to triage things here so, just ping me when you want review(s)!

import things in a bad way and they aren't tied into the top level flake so we can use things like nix flake check. Is this what you were planning?

Yup, exactly. I can take care of this if you'd like!

Thanks again for all the help!

djacu commented 2 years ago

Yup, exactly. I can take care of this if you'd like!

I might hit you up for some help. This has been a good learning opportunity for me so I'd like to try and tackle it but if I get stuck or don't think I will make good progress, I will definitely ping you.

djacu commented 2 years ago

Hey @Misterio77 I update the PR to use nixpkgs-lib so we are not dependent on nixpkgs anymore, and tested it on my system.

I updated the tests too and tried to do something like what nixpkgs does here. So we could use something like

nix-instantiate --eval --strict . -A tests.conversions

but it throws this error when I run it at the top directory in the repo

error: anonymous function at /home/bakerdn/external/nix-colors/default.nix:1:1 called without required argument 'nixpkgs-lib'

I guess that makes sense. The top level default.nix is expecting several arguments and I provided none of them. :stuck_out_tongue: You have any ideas how to remedy this or maybe a different testing setup?

djacu commented 2 years ago

@Misterio77 I pushed a new commit that fixes a bug in the test and figured out how to call the tests. Anywhere in the repo, we can use nix eval .#tests to run all the tests or nix eval .#tests.<TEST> to run a single one.

This PR is good to go unless there is anything else you want to talk about or change.

Misterio77 commented 1 year ago

Sorry for the wait!

These look great! Thanks a lot :))

Misterio77 commented 1 year ago

Done! Merging in now. Thank you :)

djacu commented 1 year ago

Glad I could help! Thanks for cleaning it up and merging it in. :)