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: cleanup flake.nix #1486

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

gytis-ivaskevicius
Copy link

What kind of change does this PR introduce?

Minor flake.nix cleanup. My editor auto removes whitespace so I would recommend enabling "hide all whitespace" option when reviewing this PR

More information on specific changes in PR comments

@gytis-ivaskevicius gytis-ivaskevicius requested a review from a team as a code owner March 20, 2025 19:41
@@ -4,12 +4,11 @@
inputs = {
nixpkgs.url = "github:nixos/nixpkgs/nixpkgs-unstable";
flake-utils.url = "github:numtide/flake-utils";
nix2container.url = "github:nlewo/nix2container";
Copy link
Author

Choose a reason for hiding this comment

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

Unused dependency

flake.nix Outdated
Comment on lines 400 to 411
inherit
wal-g
sfcgal
supabase-groonga
postgresql_15
postgresql_orioledb-17;
inherit (pkgs.cargo-pgrx)
cargo-pgrx_0_11_3
cargo-pgrx_0_12_6
cargo-pgrx_0_12_9;
Copy link
Author

Choose a reason for hiding this comment

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

Preferring the inherit statement is a good practice, also it avoids typos

flake.nix Outdated
Comment on lines 604 to 605
# TODO: None of these tests are used
sqlTests = ./nix/tests/smoke;
Copy link
Author

Choose a reason for hiding this comment

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

I am not sure if tests are failing or what, but this should be fixed somehow

@samrose
Copy link
Collaborator

samrose commented Mar 21, 2025

Thanks this is a helpful review!

@samrose
Copy link
Collaborator

samrose commented Mar 21, 2025

Everything looks good. the smoke tests are redundant and the line and files should just be removed.

@gytis-ivaskevicius gytis-ivaskevicius requested a review from a team as a code owner March 21, 2025 16:50
@gytis-ivaskevicius
Copy link
Author

Removed the smoke tests and rebased to fix merge conflicts

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