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.

Sign up for GitHub

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
+35 −4
Merged

Support pipe operators #227

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c6f39e
Support pipe operators
rhendric Jul 25, 2024
55feeb5
Rename pipes to Forward and Backward
dasJ Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/Nixfmt/Parser.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import Data.Maybe (fromMaybe, mapMaybe, maybeToList)
import Data.Text (Text)
import qualified Data.Text as Text
import Data.Void (Void)
import Nixfmt.Lexer (lexeme, pushTrivia, takeTrivia, whole)
import Nixfmt.Lexer (lexeme, takeTrivia, whole)
import Nixfmt.Parser.Float (floatParse)
import Nixfmt.Types (
Ann (..),
Expand All @@ -38,7 +38,6 @@ import Nixfmt.Types (
StringPart (..),
Term (..),
Token (..),
Trivium (..),
Whole (..),
operators,
tokenText,
Expand Down Expand Up @@ -470,7 +469,7 @@ operator t =
TMinus -> ">"
TMul -> "/"
TDiv -> "/*"
TLess -> "="
TLess -> "=|"
TGreater -> "="
TNot -> "="
_ -> ""
Expand Down
8 changes: 7 additions & 1 deletion src/Nixfmt/Types.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,8 @@ data Token
| TLessEqual
| TNot
| TUnequal
| TPipeTo
| TPipeFrom
Copy link
Member

@piegamesde piegamesde Jul 26, 2024 •
edited
Loading

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

| SOF
deriving (Eq, Show)

Expand Down Expand Up @@ -466,7 +468,9 @@ operators =
],
[Op InfixL TAnd],
[Op InfixL TOr],
[Op InfixL TImplies]
[Op InfixL TImplies],
[Op InfixL TPipeTo],
[Op InfixR TPipeFrom]
]

tokenText :: Token -> Text
Expand Down Expand Up @@ -519,4 +523,6 @@ tokenText TLess = "<"
tokenText TLessEqual = "<="
tokenText TNot = "!"
tokenText TUnequal = "!="
tokenText TPipeTo = "|>"
tokenText TPipeFrom = "<|"
tokenText SOF = ""
13 changes: 13 additions & 0 deletions test/diff/operation/in.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,4 +156,17 @@
z = 30;
}
)

# Experimental pipe operators
(
a // b
|> f "very long argument should justify splitting this over multiple lines"
|> g { }
)

(
g { } <|
f "very long argument should justify splitting this over multiple lines" <|
a // b
)
]
13 changes: 13 additions & 0 deletions test/diff/operation/out.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,4 +273,17 @@
z = 30;
}
)

# Experimental pipe operators
(
a // b
|> f "very long argument should justify splitting this over multiple lines"
|> g { }
)

(
g { }
<| f "very long argument should justify splitting this over multiple lines"
<| a // b
)
Comment on lines +284 to +288
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).

]