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

Support pipe operators #227

Merged
merged 2 commits into from
Aug 6, 2024
Merged

Conversation

rhendric
Copy link
Member

NixOS/nix#11131 landed 🎉

Let the debates about the proper formatting of these particular operators begin! Should they use the same rules as all other operators? This PR assumes yes, but there's an argument for sticking <| at the ends of lines rather than at the beginnings, since it is the final value in a chain of <|s that is special. 🤷

Copy link

github-actions bot commented Jul 25, 2024

Nixpkgs diff

Copy link
Member

@piegamesde piegamesde left a comment

Choose a reason for hiding this comment

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

My intuition also says that putting the backpipes to the right might be beneficial, but until I've written any actual code to get a feeling for it I currently don't care. And this is undoubtedly the simpler implementation

@@ -424,6 +424,8 @@ data Token
| TLessEqual
| TNot
| TUnequal
| TPipeTo
| TPipeFrom
Copy link
Member

@piegamesde piegamesde Jul 26, 2024

Choose a reason for hiding this comment

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

Small nit: I'd prefer PipeRight and PipeLeft (or PipeForward/PipeBackwards) as names, as they're a bit more visual.

Copy link
Member

Choose a reason for hiding this comment

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

Done by @dasJ in 55feeb5

Comment on lines +284 to +288
(
g { }
<| f "very long argument should justify splitting this over multiple lines"
<| a // b
)
Copy link
Member

Choose a reason for hiding this comment

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

What I like about the |> formatting is that the functions, that have an equal role, are formatted equally.
This makes it easy to read.

That is not the case here. This formatting makes it seem as if g { } is special and f "..." and a // b are similar, while the opposite is true.

Suggested change
(
g { }
<| f "very long argument should justify splitting this over multiple lines"
<| a // b
)
(
g { } <|
f "very long argument should justify splitting this over multiple lines" <|
a // b
)

A similar effect could be achieved by indenting the first function, but that's a little unusual, and it doesn't make the function operands look quite as equal.

Suggested change
(
g { }
<| f "very long argument should justify splitting this over multiple lines"
<| a // b
)
(
g { }
<| f "very long argument should justify splitting this over multiple lines"
<| a // b
)

Copy link
Member

Choose a reason for hiding this comment

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

The first suggested style has the problem that lines might be arbitrarily long, which makes it unclear how the lines relate to, without looking to the right. This is why the standard always puts operators in front.

The second suggested style violates the indentation rule of the standard.

But we also agree that it's not ideal for the first function to look different. For now we don't want to block on this and just go ahead with this PR as is.

It would also be possible for NixOS/rfcs#148 to specify how it should be formatted (though it doesn't actually specify a <| operator right now).

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

(reviewed during the formatting team meeting)

Comment on lines +284 to +288
(
g { }
<| f "very long argument should justify splitting this over multiple lines"
<| a // b
)
Copy link
Member

Choose a reason for hiding this comment

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

The first suggested style has the problem that lines might be arbitrarily long, which makes it unclear how the lines relate to, without looking to the right. This is why the standard always puts operators in front.

The second suggested style violates the indentation rule of the standard.

But we also agree that it's not ideal for the first function to look different. For now we don't want to block on this and just go ahead with this PR as is.

It would also be possible for NixOS/rfcs#148 to specify how it should be formatted (though it doesn't actually specify a <| operator right now).

@infinisil infinisil enabled auto-merge August 6, 2024 18:35
@infinisil infinisil merged commit 08144b4 into NixOS:master Aug 6, 2024
2 checks passed
@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-08-06/50222/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.

6 participants