nvim-lspconfig icon indicating copy to clipboard operation
nvim-lspconfig copied to clipboard

Minor cleanup of flake.nix

Open Rexcrazy804 opened this issue 2 years ago • 10 comments

used with pkgs; to simplify statements referring to it in the scope of mkshell. Restructured input declaration to follow the typical structure.

I am unsure whether I should mark this with doc: or not, sorry

Rexcrazy804 avatar Apr 15 '24 16:04 Rexcrazy804

If permitted, I'd like to add a shell hook that executes the default shell using the $SHELL env var so running nix develop will drop you into whatever shell you are using instead of defaulting to bash. I suppose that could be another pr?

Rexcrazy804 avatar Apr 15 '24 16:04 Rexcrazy804

@teto

justinmk avatar Apr 17 '24 14:04 justinmk

If permitted, I'd like to add a shell hook that executes the default shell using the $SHELL env var so running nix develop will drop you into whatever shell you are using instead of defaulting to bash. I suppose that could be another pr?

I disagree with this, perhaps check out direnv if you haven't already?

itslychee avatar Apr 17 '24 16:04 itslychee

@itslychee I have specified the three major systems, do you reckon there's anything else to add?

Rexcrazy804 avatar Apr 17 '24 17:04 Rexcrazy804

@itslychee I have specified the three major systems, do you reckon there's anything else to add?

Don't forget to change buildInputs to packages, and after that I'd squash all the commits into one commit to keep the history clean.

itslychee avatar Apr 17 '24 17:04 itslychee

Also since eachDefaultSystem is used currently, it's best to add "x86_64-linux" to the platform list to keep it backwards compatible as eachDefaultSystem used 4 platforms as described here

itslychee avatar Apr 17 '24 18:04 itslychee

Could you clarify that a bit further? The platforms list does contain "x86_64-linux" but it has come to my attention that "x86_64-darwin" is not included in the list, I suppose that is the one I should add.

Rexcrazy804 avatar Apr 17 '24 18:04 Rexcrazy804

Could you clarify that a bit further? The platforms list does contain "x86_64-linux" but it has come to my attention that "x86_64-darwin" is not included in the list, I suppose that is the one I should add.

Oops yeah that's the one, something possessed me to write linux instead :sob:

itslychee avatar Apr 17 '24 18:04 itslychee

I get that one would prefer to remove the dependency to flake-utils but what's the point if you have to replace allDefaultSystems by actually typing the defaults yourself ? The handling of system is one of the controversial point of flakes and nothing in this PR is objectively better so it's just churn. I would just merge an update to the lockfile to get a more recent version of the dependencies.

teto avatar May 01 '24 12:05 teto

what's the point if you have to replace allDefaultSystems by actually typing the defaults yourself

The point here is to remove a dependency that can be easily replaced with functions of nixpkgs' library, and to remove noise from the lockfile. It also makes it more clear how outputs are defined instead of having flake-utils magically insert system between a nested attrset that may not even be desirable.

The handling of system is one of the controversial point of flakes and nothing in this PR is objectively better so it's just churn.

How is eachDefaultSystem not churn then? How is removing an input that's not even needed and replacing the functionality with a genAttrs wrapper while retaining the same behavior with the previous solution not a better approach, as it also gets rid of the rec keyword that's not even used anywhere?

itslychee avatar May 01 '24 18:05 itslychee