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

[WIP] Sort assignments in sets and "let" #61

Closed
wants to merge 1 commit into from

Conversation

KAction
Copy link

@KAction KAction commented Feb 19, 2020

Sort alphabetically identifier keys in sets and let abstraction. More
complex ways to specify keys, like "inherit" statement or interpolated
sorts after plain identifier keys, but without any meaningful order
between themself.

With this change expression "{ b = 10; a = 15; }" transformed into
"{ a = 15; b = 10; }", as indented. Things are getting trickier with
comments.

Nixfmt preserves comments by attaching them to tokens before comment.
It may result to unexpected behaviour in following quite natural case:

{
  b = 12; # talk about "b"

  # This is comment about "a"
  a = 10;
}

This expression gets transformed into following, since from Nixfmt point
of view both comments are attached to "b = 12;" assignment (semicolon,
to be more precise).

{
  a = 10;
  b = 12; # talk about "b"

  # This is comment about "a"
}

DISCUSSION: What should be done about this unexpected behaviour with
comments?

First option is just make this feature opt-in. Second one, probably
harder, it to use some heuristics on whether comment should be attached
to token before or after comment. This is the most complex case I
managed to invent:

FunctionFoo { # comment on set. Dont't move!
  # comment on b, move with b.
  b = 42; # this is also comment on b.

  # comment on a.
  a = 14;

  c = [ # comment on array
    1
    2
    3
  ]; # comment on c;
}

Seems that rule "comment that takes whole line is attached to token
after it, otherwise it is attached to token before it" is good
approximation of human perception.

Dear maintainer, what do you suggest and implementation of what approach
you are willing to accept?

Sort alphabetically identifier keys in sets and let abstraction. More
complex ways to specify keys, like "inherit" statement or interpolated
sorts after plain identifier keys, but without any meaningful order
between themself.

With this change expression "{ b = 10; a = 15; }" transformed into
"{ a = 15; b = 10; }", as indented. Things are getting trickier with
comments.

Nixfmt preserves comments by attaching them to tokens before comment.
It may result to unexpected behaviour in following quite natural case:

	{
	  b = 12; # talk about "b"

	  # This is comment about "a"
	  a = 10;
	}

This expression gets transformed into following, since from Nixfmt point
of view both comments are attached to "b = 12;" assignment (semicolon,
to be more precise).

	{
	  a = 10;
	  b = 12; # talk about "b"

	  # This is comment about "a"
	}
@Lucus16
Copy link
Contributor

Lucus16 commented Feb 19, 2020

Thank you for the suggestion and for helping out! I have the following thoughts:

First of all, sorting can never be default in a formatter because it is not always what people want. There are two acceptable ways to turn it on, either with a flag that turns it on for everything or with a comment like # nixfmt: sort attached to the specific structure one wants to sort. This shouldn't be much trouble to implement though.

Second, you've correctly identified the issue with comments. As you said, ideally a trailing comment is attached to the token before it on the same line and all comments on their own line are attached to the token that follows them. I'd like to this while parsing already but I couldn't make that work. I forgot the details, but I think it affected how parsers failed and therefor whether the next alternative would be tried or not, so it would accept a different language.

Since I couldn't figure out how to solve it during parsing, a transformation pass that moves all the comments to the correct token would be good. Either way, the formatting of many things needs to be fixed after that. To complicate things, the formatter sometimes moves comments to different tokens, e.g. past a comma. Those moves should also become part of the same transformation. Issue #32 tracks this, but I haven't spent time on it yet. I'll see if I can make some time for it soon.

In short, the bulk of the hard work is in fixing #32, after that, this shouldn't be a problem to implement.

@infinisil
Copy link
Member

(discussed in the formatting team meeting today)

While we think sorting attribute sets could be a good opt-in feature, we'd first need to figure out an opt-in mechanism. Let's close this PR for now since it's a bit outdated.

@infinisil infinisil closed this Apr 2, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/formatting-team-meeting-2024-04-02/42658/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants