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

Format with alejandra #453

Closed
wants to merge 2 commits into from
Closed

Format with alejandra #453

wants to merge 2 commits into from

Conversation

Pacman99
Copy link
Member

@Pacman99 Pacman99 commented Apr 21, 2022

alejandra seems like the best choice out there and our codebase would look a lot cleaner with this.

This should probably be merged after #438, so we don't have to deal with the conflicts.

The first commit is me, the second commit is all alejandra.

nix 2.8 isn't in nixpkgs yet, but you can always do nix develop -c fmt

@Pacman99 Pacman99 marked this pull request as draft April 21, 2022 04:09
@Pacman99 Pacman99 force-pushed the formatting branch 2 times, most recently from d6eff0f to a05017f Compare April 21, 2022 18:30
@Pacman99 Pacman99 changed the title Formatting Format with alejandra Apr 21, 2022
@Pacman99 Pacman99 force-pushed the formatting branch 2 times, most recently from 204da4f to 41ac43c Compare April 21, 2022 18:39
@montchr
Copy link
Collaborator

montchr commented Apr 22, 2022

I haven't yet used https://github.com/numtide/treefmt, but it looks pretty useful as "One CLI to format the code tree". @blaggacao originally introduced its usage in the devshell changes reverted in #444. Looks like it could handle formatting for *.nix files with alejandra, in addition to other formatters per file type.

Considering formatting changes are usually pretty disruptive, it might be worth resurrecting the usage of treefmt in this PR since the treefmt.toml file there also added formatter configuration for *.sh files with shfmt, and we have a few of those in the repo. Plus, Prettier for all the rest.

https://github.com/divnix/digga/pull/444/files#diff-83489924125e52fef286509dc792070b570a0c656cd9b048445f87bebc57405a

Comment on lines +100 to +102
value = '' extra-experimental-features = nix-command flakes
extra-substituters = https://nrdxp.cachix.org https://nix-community.cachix.org
extra-trusted-public-keys = nrdxp.cachix.org-1:Fc5PSqY2Jm1TrWfm88l6cvGWwz3s93c6IOifQWnhNW4= nix-community.cachix.org-1:mB9FSh9qf2dCimDSUo8Zy7bkq5CX+/rkCWyvRCYg3Fs='';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
value = '' extra-experimental-features = nix-command flakes
extra-substituters = https://nrdxp.cachix.org https://nix-community.cachix.org
extra-trusted-public-keys = nrdxp.cachix.org-1:Fc5PSqY2Jm1TrWfm88l6cvGWwz3s93c6IOifQWnhNW4= nix-community.cachix.org-1:mB9FSh9qf2dCimDSUo8Zy7bkq5CX+/rkCWyvRCYg3Fs='';
value = ''
extra-experimental-features = nix-command flakes
extra-substituters = https://nrdxp.cachix.org https://nix-community.cachix.org
extra-trusted-public-keys = nrdxp.cachix.org-1:Fc5PSqY2Jm1TrWfm88l6cvGWwz3s93c6IOifQWnhNW4= nix-community.cachix.org-1:mB9FSh9qf2dCimDSUo8Zy7bkq5CX+/rkCWyvRCYg3Fs=
'';

Comment on lines +106 to +119
# tempfix: remove when merged https://github.com/numtide/devshell/pull/123
devshell.startup.load_profiles = pkgs.lib.mkForce (pkgs.lib.noDepEntry ''
# PATH is devshell's exorbitant privilige:
# fence against its pollution
_PATH=''${PATH}
# Load installed profiles
for file in "$DEVSHELL_DIR/etc/profile.d/"*.sh; do
# If that folder doesn't exist, bash loves to return the whole glob
[[ -f "$file" ]] && source "$file"
done
# Exert exorbitant privilige and leave no trace
export PATH=''${_PATH}
unset _PATH
'');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been eyeing this for a bit: is it still necessary? Looks like numtide/devshell#123 is fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'm not entirely sure about this either. I'll remove it and check if anything breaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is fixed. Thx!

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed in #490.

@nrdxp
Copy link
Collaborator

nrdxp commented Apr 22, 2022

Since treefmt was mentioned, we have a pretty useful solution at work that uses a devshell pre-commit hook to call treefmt on the entire repo before any commits are made. I don't think anybody would mind if we steal it:
https://github.com/input-output-hk/devshell-capsules/blob/main/pre-commit.sh

I know we had something very similar here before, not sure if we still do

@Pacman99
Copy link
Member Author

Definitely, I'll setup treefmt here.

We still have the pre-commit hook for devos, but it was never added to digga's devshell, I can do that here to. And we can switch to treefmt in the hook too. Right now it uses nixpkgs-fmt.

@Lord-Valen
Copy link
Contributor

This can be closed since #491 was merged.

@Pacman99 Pacman99 closed this Nov 12, 2022
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.

5 participants