RFC 101/166 style #118

piegamesde · 2023-04-19T09:36:21Z

This is the experimental branch where we try to implement currently discussed formatting decisions, to get a better feeling of how they affect real-world code.

To make more clear that one formats and the other one only checks

Tests mostly imported from Alejandra

- Disallow single-line if statements - Handle multi-line conditions differently (indent instead of absorb)

- The (from) case needs to be improved - Somehow it indents the comments *after* the statement now, no idea why or how to fix

The file was identical to diff/idioms_nixos_1 and therefore redundant. Moreover, we need to also keep a look on the *really* big packages in there, and Mozilla is a prime example.

Except when that item is another list or attribute set

This is nixos/modules/services/web-apps/nextcloud.nix, so that we have a big NixOS module in our suite

infinisil · 2023-05-08T15:39:26Z

There's a bug in this PR that indents comments too much:

Input

let
  a = null;
  # b
in
null

Output

let
  a = null;
    # b
in
null

piegamesde · 2023-05-08T15:57:48Z

No it's not a bug in this PR, it's #32 / #117. However, the style changes in this PR make the issue a lot more prominent. I currently ignore all comment formatting for now until #32 is fixed.

toastal · 2024-03-06T10:34:07Z

.envrc

@@ -0,0 +1 @@
+use_nix


Why is this added? Should it be up to the user whether or not they would want to use this specific default Nix shell? Committing this to the repo means anyone with direnv installed is going to get that big red consent message after cd-ing into the project for the first time. This also makes this hard to extend if users have other environment needs & now have to deal with potentially accidentally committing those changes as this personal environment file is now checked into the repository.

I would prefer this be moved to .envrc.example but given it is a single line & those with direnv already know how to use it, a line could be added to the README or flake that says enabling is echo "use flake" > .envrc && direnv allow.

What about direnv deny? Also works with direnv disallow, direnv block, and direnv revoke

I honestly don't understand your point. Committing the .envrc file is the point of direnv, to make the developer experience smoother. I love it when projects do that. The shell is additive and not pure, so it will take the outside environment into account.

direnv deny

This isn’t the experience a user wants. They want to enable direnv, but only after they have a) had time to inspect the .envrc file & b) made modifications for themselves. Current state throws up ANSI red prompt just for cd-ing into the directory. Allow should happen after everything is ready.

the point

Scenario: You and I have different setup needs. I need different environment variables & scripts specific to me. The only way to get them for my personal setup is to edit the .envrc. Now that I have edited the .envrc, git status is saying I have made modification even tho they should be specific to me. I might also accidentally commit those changes as a result since it’s a tracked file. I believe the point is to set up your environment for you, not for everyone as not everyone has the same environment. The point of an .envrc.example is the place for a suggested setup for the average user. cp .envrc.example .envrc is one tiny step. Many projects have .env in their .gitignore because they don’t want to a) check in sensitive env info b) prescribe everyone have the same setup. This is as weird as committing your personal text editor settings… they are your environment settings, not everyone else’s.

love it

I hate it. If I just need to edit a README, I don’t want a direnv prompt, & certainly don’t want to copy down another version of Nixpkgs with tooling I won’t be needing for my task, and definitely not automatically running arbritrary executables I don’t yet understand because I just cloned the repo. I do like it when they offer an example I can copy, or if there is nothing interesting but use flake or use nix, just stick it in the docs.

This isn’t the experience a user wants.

As a user, I expect that when I install a tool (direnv) and a repo supports it (nixfmt), then the tool should be used. Don't want direnv? Don't install it, then.

I need different environment variables & scripts specific to me

Great, direnv has support for that: $XDG_CONFIG_HOME/direnv/direnvrc and $XDG_CONFIG_HOME/direnv/lib/*.sh. We could also do this but that has security implications.

To be honest, I have never heard from any developer who has direnv installed that they don't want to use it (why did they install it then?). To me it's as simple as:

Only editing the readme once? Ignore the warning

Doing something in the repo often? direnv deny it once. It's quick and easy

Doing something in the repo often but you are like me? Ignore the warning every time

Also, please don't block this PR on that, I think everyone can handle whatever outcome we end up with 🙈 Thank you for your work!! :)

To be honest, I have never heard from any developer who has direnv installed that they don't want to use it (why did they install it then?). To me it's as simple as:

Only editing the readme once? Ignore the warning

Doing something in the repo often? direnv deny it once. It's quick and easy

Doing something in the repo often but you are like me? Ignore the warning every time

@dasJ Late to the party, but I just wanted to say I strongly discourage adding .envrc to a git repo (or any other VCS), and it's not about having direnv installed and not wanting to use it btw :) Here are some counter arguments:

I want to have my own nix shell and env vars when working on this repo (or any other), for which I would like to be in control of my own .envrc file.

Specifically on this project, what if I want to use a different Haskell LSP server version? or a different Cabal version? Or if I need some other package in this environment that the project assumes it's globally available?

I want to be able to source other local files relevant to my environment on that .envrc, e.g. I use fish shell and it requires some tweaks on certain repos, other folks use zsh or bash, etc.

Having this file committed to the repo removes this flexibility, but it's also easier to argue in its favor for an open source public repo. For private repos there's even more counter arguments, for instance, we have this at work:

Need to set AWS secrets on a daily basis (lasts 24 hours).

Need to set specific secrets that last a year (can be sourced from a local file).

Still need specific Nix shells at times (every project is different).

Need project-specific env vars that need to change depending on what we are testing and on which environment.

What I suggest instead

Let the users decide what they want available in their environments while still leveraging direnv, be a good citizen and add .envrc to your global .gitignore or add it to the project's .gitignore to avoid anyone adding it back.

Or if I need some other package in this environment that the project assumes it's globally available?

a good project shell would assume no such things

Regarding credentials: these should not be set in .envrc, but project shell should provide tools (scripts, commands) to manage these

Still need specific Nix shells at times (every project is different).

These can be entered ad-hoc with nix shell\develop

Need project-specific env vars that need to change depending on what we are testing and on which environment.

A command (provided by default dev shell) should be provided to switch between envs

.envrc IMHO should never be edited by devs, it should simply setup all tools needed to work on given project. If customisation of env vars is needed this could be loaded from optional, gitignored file(s)

a good project shell would assume no such things

This project assumes I need a specific HLS version

Regarding credentials: these should not be set in .envrc, but project shell should provide tools (scripts, commands) to manage these

Agree to disagree. The methods vary across different projects when working on a big organization. A lot of flows are very complex and unfortunately can't be automated, e.g. sign in via SSO behind a VPN, approving the sign in attempt via Okta / Duo, etc. It barely works on Linux, let alone trying to automate these things.

It's much easier on open source public projects, I'll give you that, but private organizations are a completely different story.

These can be entered ad-hoc with nix shell\develop

It doesn't make it easier to pin specific pkgs, add overlays, etc.

A command (provided by default dev shell) should be provided to switch between envs

This isn't bad actually, might work well in some cases, will give it a try :) Certainly not in cases where switching envs requires going through the complex authorization flows, though.

If customisation of env vars is needed this could be loaded from optional, gitignored file(s)

How would you do this if you don't control the content of .envrc?

We've had this topic at least thrice already. Not sure having this discussion yet another time is a productive use of anybody's time

DavHau · 2024-03-07T09:55:12Z

Found a little weirdness here, while formatting my code base:

in:

(import ./default.nix).devShells.${builtins.currentSystem}.default
or (throw "dev-shell not defined. Cannot find flake attribute devShell.${builtins.currentSystem}.default")

out:

(import ./default.nix).devShells.${builtins.currentSystem}.default
  or (throw "dev-shell not defined. Cannot find flake attribute devShell.${builtins.currentSystem}.default"
  )

piegamesde · 2024-03-07T10:41:02Z

@DavHau can you please confirm that you are running on the latest commit? This bug should have been fixed in 6bd07b5, and I cannot reproduce it anymore

DavHau · 2024-03-07T14:50:47Z

Oops, most likely this was the case. Never mind then.

And thanks to the rfc team for doing this great work. I imagine formatting discussions to be quite tiring. What you are doing is very valuable and will support large amounts of developers not having to waste their time on formatting related discussions anymore or reading badly formatted code. Thanks for pushing this through!

You are awesome!

terlar · 2024-03-08T15:30:49Z

I was just testing out this from nixpkgs together with treefmt.

«derivation /nix/store/qkgjk776s6pb7y7lzh7xxfjp86mn1v93-nixfmt-unstable-2024-03-01.drv»

Seems to be a regression where nixfmt modifies the timestamp of the files every time and thus causing treefmt to fail when run with --fail-on-change.
https://numtide.github.io/treefmt/formatters-spec/#2-write-to-changed-files

$ sha256sum flake.nix 
c4f47d1cdb986ea08343ee18791842e0969f82feaeaf8b94f5afd6ab51c994ee  flake.nix
$ ls -la flake.nix
-rw-r--r-- 1 terje terje 2829 mar  8 16:23 flake.nix
$ nix run nixpkgs#nixfmt-rfc-style flake.nix
$ sha256sum flake.nix
c4f47d1cdb986ea08343ee18791842e0969f82feaeaf8b94f5afd6ab51c994ee  flake.nix
$ ls -la flake.nix
-rw-r--r-- 1 terje terje 2829 mar  8 16:29 flake.nix

piegamesde · 2024-03-08T15:34:56Z

@terlar this is fixed on master but not here yet unfortunately: #88

infinisil

Overall this is looking okay, so instead of trying to review this in-depth, let's just merge it and iterate with further PRs!

There's still more work to do before the first official release though, see #153 for the overall progress.

infinisil · 2024-03-14T00:02:27Z

There's a test failure after the master merge, could you check that out @piegamesde?

❯ test/test.sh
nixfmt v0.6.0
[OK] test/correct/commented-list.nix
[OK] test/correct/dollars-before-interpolation.nix
[OK] test/correct/final-comments-in-sets.nix
[OK] test/correct/float-below-one.nix
[OK] test/correct/if-with-comments.nix
[OK] test/correct/paths-with-interpolations.nix
[OK] test/correct/quotes-in-inherit-2.nix
[OK] test/correct/quotes-in-inherit.nix
[OK] test/correct/short-inherit-from.nix
[OK] test/correct/standalone-comments.nix
[OK] test/correct/string-with-single-quote-at-end.nix
[OK] test/invalid/interpolation-in-env-path.nix
[OK] test/invalid/interpolation-in-inherit-1.nix
[OK] test/invalid/interpolation-in-inherit-2.nix
[OK] test/invalid/naked-interpolation.nix
[OK] test/invalid/path-starting-with-interpolation.nix
[OK] test/invalid/path-with-escaped-interpolation.nix
[OK] test/invalid/path-with-interpolation-before-slash.nix
[OK] test/invalid/smiley.nix
Checking test/diff/apply/in.nix …
--- test/diff/apply/out.nix	2024-03-14 00:41:10.594172158 +0100
+++ /dev/fd/63	2024-03-14 01:01:43.945198577 +0100
@@ -316,8 +316,7 @@
     foo2 =
       {
         # A lot of values here
-      }
-      .overrideAttrs
+      }.overrideAttrs
         (prevAttrs: {
           # stuff here
         });
[ERROR] (run with --update-diff to update the diff)

Closes NixOS#122

infinisil · 2024-03-14T19:04:35Z

Tests now pass, and I can also confirm that the formatting of Nixpkgs has not changed one bit after the master merge, so this is good to go now!

This reverts commit 5303938. [これ](https://github.com/NixOS/nixfmt/pull/118)がnixpkgsに入るまでは待つことにする。

Lucus16 added 6 commits April 6, 2023 12:20

Apply hlint suggestions

d2cc629

Generalize parsing utilities

d83a00c

Attach leading trivia to next token

c8ebd02

Add test for if-with-comments

0c9d9a4

Fix argument order in Ann

f702d73

Allow standalone comments in lists and sets

5ae8e62

infinisil mentioned this pull request May 2, 2023

[RFC 0101] Nix formatting NixOS/rfcs#101

Closed

piegamesde added 2 commits May 5, 2023 11:49

Add direnv

4a05494

Update CLI flags description

13b7904

To make more clear that one formats and the other one only checks

piegamesde force-pushed the rfc101-style branch from bd478c1 to 9698ef7 Compare May 5, 2023 11:23

piegamesde added 6 commits May 5, 2023 13:32

Add simple test runner

f938cfc

Tests mostly imported from Alejandra

Rework function declarations

9ce4ce3

Force-expand lists with more than one item

e05d6a2

Expand let statements more

305ed77

Rework if statements

73e7ec4

- Disallow single-line if statements - Handle multi-line conditions differently (indent instead of absorb)

Don't indent in body anymore

cd0acd3

piegamesde force-pushed the rfc101-style branch from 9698ef7 to 818f63c Compare May 5, 2023 12:29

piegamesde added 3 commits May 5, 2023 16:39

Rework inherit statements

8315ca3

- The (from) case needs to be improved - Somehow it indents the comments *after* the statement now, no idea why or how to fix

fixup! Rework if statements

28a54b3

Add some code documentation

7fe6a99

piegamesde force-pushed the rfc101-style branch from 818f63c to 7fe6a99 Compare May 5, 2023 14:39

Rework bindings

e9cdac6

piegamesde force-pushed the rfc101-style branch from 1510f5e to e9cdac6 Compare May 5, 2023 15:52

piegamesde added 4 commits May 7, 2023 11:30

Tests: replace diff/idioms_pkgs_3

2e2797a

The file was identical to diff/idioms_nixos_1 and therefore redundant. Moreover, we need to also keep a look on the *really* big packages in there, and Mozilla is a prime example.

fixup! Rework bindings

dafec15

Expand singleton lists with a multiline item

de7bd6d

Except when that item is another list or attribute set

Test: add idioms_nixos_2

d1ba3cd

This is nixos/modules/services/web-apps/nextcloud.nix, so that we have a big NixOS module in our suite

Don't absort in body anymore

e6bed2a

piegamesde force-pushed the rfc101-style branch from 19a377b to 2b5ee82 Compare March 1, 2024 17:41

toastal reviewed Mar 6, 2024

View reviewed changes

piegamesde marked this pull request as ready for review March 6, 2024 14:36

infinisil mentioned this pull request Mar 12, 2024

RFC 166 implementation tracking issue #153

Open

15 tasks

infinisil approved these changes Mar 13, 2024

View reviewed changes

Merge branch 'master' into rfc101-style

7d99e0b

infinisil force-pushed the rfc101-style branch from 96a8ca8 to 669f03e Compare March 13, 2024 23:44

infinisil and others added 4 commits March 14, 2024 19:16

Use /usr/bin/env shebang in tests

4eb99c1

Comply with REUSE

08b34c0

Remove accidentally committed file

f43ec86

Parser: Fix operator parsing

8d13b59

Closes NixOS#122

infinisil force-pushed the rfc101-style branch from 4db5814 to 8d13b59 Compare March 14, 2024 18:16

infinisil merged commit 91b200a into NixOS:master Mar 14, 2024
1 check passed

This was referenced Mar 14, 2024

Comments following inherit are not indented correctly #126

Closed

nixfmt keeps moving comments immediately after in #100

Closed

Expansion of \t in strings #63

Closed

infinisil mentioned this pull request Mar 14, 2024

Run test suite in CI #165

Closed

kachick mentioned this pull request Mar 23, 2024

Replace nixpkgs-fmt with nixfmt kachick/anylang-template#75

Merged

Janik-Haag mentioned this pull request Apr 1, 2024

mv .envrc → .envrc.example #178

Closed

infinisil mentioned this pull request Apr 2, 2024

Configurable indentation size #77

Open

cq2n-iwym referenced this pull request in Hogeyama/nix-config Apr 5, 2024

Revert "nvim: Use nixfmt instead of nixpkgs-fmt for formatting"

0d7ec66

This reverts commit 5303938. [これ](https://github.com/NixOS/nixfmt/pull/118)がnixpkgsに入るまでは待つことにする。

notnmeyer mentioned this pull request Jun 24, 2024

set nixfmt as the default formatter for nix helix-editor/helix#11001

Closed

gvolpe mentioned this pull request Aug 15, 2024

Load .envrc.local, .envrc.dist direnv/direnv#556

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC 101/166 style #118

RFC 101/166 style #118

piegamesde commented Apr 19, 2023

infinisil commented May 8, 2023

piegamesde commented May 8, 2023

toastal Mar 6, 2024

dasJ Mar 6, 2024

piegamesde Mar 6, 2024

toastal Mar 6, 2024 •

edited

Loading

dasJ Mar 6, 2024

futile Mar 6, 2024

gvolpe Aug 15, 2024

adrian-gierakowski Aug 15, 2024 •

edited

Loading

gvolpe Aug 15, 2024

piegamesde Aug 15, 2024

DavHau commented Mar 7, 2024

piegamesde commented Mar 7, 2024

DavHau commented Mar 7, 2024

terlar commented Mar 8, 2024

piegamesde commented Mar 8, 2024

infinisil left a comment

infinisil commented Mar 14, 2024

infinisil commented Mar 14, 2024

		@@ -0,0 +1 @@
		use_nix

RFC 101/166 style #118

RFC 101/166 style #118

Conversation

piegamesde commented Apr 19, 2023

infinisil commented May 8, 2023

piegamesde commented May 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toastal Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

What I suggest instead

adrian-gierakowski Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavHau commented Mar 7, 2024

piegamesde commented Mar 7, 2024

DavHau commented Mar 7, 2024

terlar commented Mar 8, 2024

piegamesde commented Mar 8, 2024

infinisil left a comment

Choose a reason for hiding this comment

infinisil commented Mar 14, 2024

infinisil commented Mar 14, 2024

toastal Mar 6, 2024 •

edited

Loading

adrian-gierakowski Aug 15, 2024 •

edited

Loading