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

chore: convert flake to flake parts and add git-hooks #1599

Merged
merged 9 commits into from
Dec 12, 2024

Conversation

thilobillerbeck
Copy link
Collaborator

This PR adds

  • flake-parts to the flake
  • a git hook that checks formatting on push
  • npm package installation if node_modules does not exist to prevent errors when pushing

The reason I set on-push for the hooks is, that people could get quite annoyed when having to check for proper formatting on every commit. Since we doe merge PRs anyway, we can require proper formatting on push and ease local development for contributors.

I'd like to hear feedback from other contributors here on what to add or to change before I merge this into main.

Copy link
Contributor

github-actions bot commented Dec 1, 2024

Copy link
Member

@djacu djacu left a comment

Choose a reason for hiding this comment

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

I'm down with all these changes. On-push hooks sounds like a nice QoL improvement!

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

I just added minor and optional comments.

What I would do here is to format the file with nixfmt-rfc-style.

flake.nix Outdated
flake-utils.lib.eachDefaultSystem (system:
let
flake-parts.lib.mkFlake { inherit inputs; } {
flake = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the parts we don't need

flake.nix Outdated
imports = [
inputs.git-hooks-nix.flakeModule
];
systems = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@crertel crertel left a comment

Choose a reason for hiding this comment

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

LGTM.

@thilobillerbeck thilobillerbeck merged commit 0a8b14e into main Dec 12, 2024
2 checks passed
@thilobillerbeck thilobillerbeck deleted the flake-parts branch December 12, 2024 00:47
Copy link
Contributor

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

I might submit a PR to fix this and some other minor improvements.

imports = [
inputs.git-hooks-nix.flakeModule
];
systems = import systems;
Copy link
Contributor

Choose a reason for hiding this comment

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

{ config, system, ... }:
let
overlay = final: prev: {
asciinema-scenario = final.rustPlatform.buildRustPackage rec {
Copy link
Contributor

Choose a reason for hiding this comment

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


pkgs = import nixpkgs { inherit system; overlays = [ overlay ]; };
inherit (pkgs.lib) getVersion;
pkgs = import nixpkgs {
Copy link
Contributor

Choose a reason for hiding this comment

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

pkgs is already provided by the flake.parts framework, use it as such: https://flake.parts/overlays#consuming-an-overlay

@thilobillerbeck
Copy link
Collaborator Author

I might submit a PR to fix this and some other minor improvements.

That'd be great, I thinks the flake can see some improvements anyway. I'm pretty new to flake-parts though, so it's good to have some people around who have used it for longer :D

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.

4 participants