I was wondering if the RFC 166 implementers are interested in publishing a survey regarding the behavior of the new nixfmt. I personally have at least 1 dissatisfaction with it, and I was wondering how many people feel like me.
To be specific, my dissatisfaction is from this code:
Which is a much bigger diff, due to the indentation change. Similar examples can be produced for multi-line strings and lib.optionalString usages. In case of large multi-line strings, these diffs are too large IMO, and I’m not sure I understand what is satisfied by this behavior.
Of course this is only 1 example, other users my have experienced surprises from other formatting rules, and I think it’d be interesting to iterate this after feedback from the community, which may hadn’t had time to delve and be subscribed to all of the discussions of the original RFC.
Would love to see a survey regarding this which could potentially even help spawn a new RFC to iron things out, and helpful to see in either scenario.
I also have the exact same dissatisfaction with the new formatter’s behaviour; and it applies to instances other than lists (like functions), which lead to a decently sized diff in my flake.
Personally, I’m not sure what its goal would be and if it would be worth the effort doing. By now, I’ve heard every opinion on every feature, and for every request I’ve seen someone else request the opposite. The decisions in Nixfmt try to make balanced tradeoffs for different situations.
Because the truth is, there are some situations where one option is better and others where the opposite is better. And then people look at a situation and say “I’d like it better the other way around” without seeing all the ways things would get worse elsewhere.
So if we really want to make progress, the goal should be to fine-tune the rules with better heuristics to do the Right Thing hopefully most of the time.
I think that’s exactly what a survey would provide you insight about - it’d tell you which rules are more important to people then others. I understand there are some rules which some people strongly agree with and some people strongly disagree with, and I do think that people who actually develop/maintain nixfmt and that were involved in the RFC should eventually reach a decision, but perhaps such a survey would give them at least a small hint about what would satisfy the community a bit.
Another thing that can grow out of such a survey, is list of common queries about the rules, and a reply to each of them. This could somewhat satisfy some users who are currently upset about the rules.
My personal thought is the community spent a year on nitting the RFCs (166 and 101) and I don’t see a practical benefit to reformatting churn. Unless there is really a strong (readability-related) case for one way of formatting, it’s primarily just esthetics, and as ugly as it is, people need to get over it and worry about our actual nixpkgs issues.
Wasn’t the whole purpose of having an official formatter to avoid the nitting, not just move it elsewhere?
Actually on that note, I’ve noticed one class of nit that still seems to come through even after nixfmt-ing: addition of whitespace lines for readability (e.g. between, say, version and src, or src and whatever comes after, if ordered per the informal standards). Is there a way to enforce that whitespace (or lack thereof)?
If I had to formalise the rule, I’d say “whitespace lines around attributes that have attrsets, multiline lists, and indented strings”.
What would personally help me instead, in terms of community feedback, is to see in-context formatting which people find dissatisfying. This is a lot more helpful than an “I don’t like this” formatting example out of context. (I know, this is a bit counter-intuitive to minimizing reproducers which would normally be desirable.) Having more context will help tweak the rules for these specific cases without regressing everything else.
I’m not sure I understand what is the difference between out of context v.s in-context complaints. Would you consider @Sandro 's issue an example of in-context complaint?
here’s an example that i think can be improved on. the non-nixfmt is linked from git hub and nixfmt is pasted below. the issue i see is that the items in the list are all aligned on the left in the non-nixfmt version but when formatted jump around to satisfy the various formatting rules.
cxxCMakeFlags =
[ "-DLIBCXX_CXX_ABI=${cxxabiName}" ]
++ lib.optionals (cxxabi == null && lib.versionAtLeast release_version "16") [
# Note: llvm < 16 doesn't support this flag (or it's broken); handled in postInstall instead.
# Include libc++abi symbols within libc++.a for static linking libc++;
# dynamic linking includes them through libc++.so being a linker script
# which includes both shared objects.
"-DLIBCXX_STATICALLY_LINK_ABI_IN_STATIC_LIBRARY=ON"
]
++ lib.optionals (cxxabi != null) [ "-DLIBCXX_CXX_ABI_INCLUDE_PATHS=${lib.getDev cxxabi}/include" ]
++ lib.optionals (stdenv.hostPlatform.isMusl || stdenv.hostPlatform.isWasi) [
"-DLIBCXX_HAS_MUSL_LIBC=1"
]
++ lib.optionals (
lib.versionAtLeast release_version "18"
&& !useLLVM
&& stdenv.hostPlatform.libc == "glibc"
&& !stdenv.hostPlatform.isStatic
) [ "-DLIBCXX_ADDITIONAL_LIBRARIES=gcc_s" ]
++ lib.optionals (lib.versionAtLeast release_version "18" && stdenv.hostPlatform.isFreeBSD) [
# Name and documentation claim this is for libc++abi, but its man effect is adding `-lunwind`
# to the libc++.so linker script. We want FreeBSD's so-called libgcc instead of libunwind.
"-DLIBCXXABI_USE_LLVM_UNWINDER=OFF"
]
++ lib.optionals useLLVM [ "-DLIBCXX_USE_COMPILER_RT=ON" ]
++ lib.optionals (
useLLVM && !stdenv.hostPlatform.isFreeBSD && lib.versionAtLeast release_version "16"
) [ "-DLIBCXX_ADDITIONAL_LIBRARIES=unwind" ]
++ lib.optionals stdenv.hostPlatform.isWasm [
"-DLIBCXX_ENABLE_THREADS=OFF"
"-DLIBCXX_ENABLE_FILESYSTEM=OFF"
"-DLIBCXX_ENABLE_EXCEPTIONS=OFF"
]
++ lib.optionals (!enableShared) [ "-DLIBCXX_ENABLE_SHARED=OFF" ]
++ lib.optionals (cxxabi != null && cxxabi.libName == "cxxrt") [
"-DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=ON"
];
I haven’t been following RFC 166 at all and I have just noticed the formatting check in nixpkgs CI. I really hope the plan is not to bulk-reformat the entirety of Nixpkgs, particulary NixOS modules where the complexity is higher.
I don’t object to any styling decision, because it’s all arbitrary anyway, but objectively nixfmt code uses way too many line breaks and indentation. For example, lists
- {
- x = [ 1 2 3 ];
+ x = [
+ 1
+ 2
+ 3
+ ];
}
or string interpolation:
- ${if iface == null then ''
- # detect interfaces automatically
+ ${
+ if iface == null then
+ ''
+ # detect interfaces automatically
When you have a bunch of this in the same snippet of code it explodes:
- assertions = flip mapAttrsToList cfg.networks (name: cfg: {
- assertion = with cfg; count (x: x != null) [ psk pskRaw auth ] <= 1;
- message = ''options networking.wireless."${name}".{psk,pskRaw,auth} are mutually exclusive'';
- }) ++ [
- {
- assertion = length cfg.interfaces > 1 -> !cfg.dbusControlled;
- message =
- let daemon = if config.networking.networkmanager.enable then "NetworkManager" else
- if config.services.connman.enable then "connman" else null;
+ assertions =
+ flip mapAttrsToList cfg.networks (
+ name: cfg: {
+ assertion =
+ with cfg;
+ count (x: x != null) [
+ psk
+ pskRaw
+ auth
+ ] <= 1;
+ message = ''options networking.wireless."${name}".{psk,pskRaw,auth} are mutually exclusive'';
+ }
+ )
+ ++ [
+ {
+ assertion = length cfg.interfaces > 1 -> !cfg.dbusControlled;
+ message =
+ let
+ daemon =
+ if config.networking.networkmanager.enable then
+ "NetworkManager"
+ else if config.services.connman.enable then
+ "connman"
+ else
+ null;
I’d rather have minor styling inconsistencies than code with 8-10 indentation levels.
While I can certainly see where you’re coming from, the trade-off between consistency and vertical aswell as horizontal space usage was made intentionally.
The time to provide such feedback was months ago when the RFC was still open and discussed though. Reference reformats of Nixpkgs were provided for pretty much the entire time I can remember the RFC being worked on. There were multiple quite public calls for testing. Calling such fundamental trade-off decisions of the RFC into question now is not really appropriate anymore IMO.
I can’t speak for the formatting team but your (IMHO valid) concerns of yours will likely have to be addressed after the reformat has happened. We’ll have to live with slightly suboptimal but consistent formatting in the meantime and I think that’s still a net positive.
Allowing short lists or attrsets to remain on a single line requires the formatter to be dependent upon the input formatting rather than being a pure function of the AST. IIRC it was planned to add an exception for this specific special case but I’m not sure when that’s supposed to happen.
I’m not sure what’s going on with the string interpolation example starting with an empty line, that does not look intended.
While I can certainly see where you’re coming from, the trade-off between consistency and vertical aswell as horizontal space usage was made intentionally.
We’ll have to live with slightly suboptimal but consistent formatting in the meantime and I think that’s still a net positive.
My impression is that this formatter was made more with packages in mind (most are slightly more complex than metadata). I agree it’s a good idea to standardise the style here due to the large number of maintainers and automatic tools handling them.
For library code and modules with (optimistically) two or three people that work on them, it seems counterproductive.
Calling such fundamental trade-off decisions of the RFC into question now is not really appropriate anymore IMO.
You’re right, but there’s been a lot going on with Nix around the same time, with much higher priority than code style. So, I would expect a lot more feedback and angry people with pitchforks to come once these changes hit Nixpkgs.
By the way, I’m not asking for anything, I’m just sharing my impressions here, which I understand is the purpose of this thread.
Merely allowing short lists to remain on a line, which is already supported by the RFC (just not the implementation), would go a long way towards fixing this particular explosion (and most other issues that I’ve seen in formatting my own code).
I think that’s a different issue; that patch lets expanded short lists remain expanded, but it still expands collapsed short lists, which is one of the complaints raised here.
Which is why it would’ve been a really bad choice. But now that expanded attribute sets are preserved, the above wouldn’t get reformatted, while also allowing the second style
That indeed is exactly the plan. However while the RFC is done and fixes a lot of the format in place, I think there still is a lot of tweaking to do on the last mile before we go for the full deployment.
This is interesting to hear, because I’ve also heard criticism that the formatter was made for complex library code in mind and does handle packages that well. In the end we looked at all kinds of code to make tradeoffs, but I’d agree that the chosen format is at its weakest with module definitions.
@Infinisil that’s great news, because this bothered me very much. I hope this update will get in as fast as possible .
In general, what bothered me the most and what made me write this post, was that I read the RFC, and I read and understood the comments I received during FCP, and all of a sudden I see what the implementation does and I don’t understand how could I have missed these rules…
Related to lists and the PR above, I still don’t understand what rules in the RFC make the following example a bad formatting (the example from my initial post):
.