-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Deprecate |> #24886
Deprecate |> #24886
Conversation
Not sure if this is fully sufficient. I think might need to be deprecated to a syntax error if we want non-function-like functionality in the future? |
This has been discussed before, and the consensus was against removing |
Recent tide of support here: #5571 (comment) |
The purpose of deprecating it as a function is to reintroduce it as syntax. |
Okay, sounds good. |
Would reintroducing it as syntax mean that you wouldn't be able to alias it, e.g. say Slightly OT, but I wonder if there's a more general mechanism that could be used, e.g. operator macros... |
Not sure if the broadcast test i deleted was important... |
Worth noting how much uglier code got without chaining (and how much nicer |
To me, the code became quite a lot clearer. |
Echoing Kristoffer, almost every rewrite in this pull request was clearer to me. Best! |
Does anyone know what these deserializing a remote exception errors mean? I probably made a mistake in the triangular and cholmod files but I don't see a clear line number |
da da da da da da daaaaaa |
This looks good to me? CI seems unusually happy |
I have to say, I don't really understand the reason for deprecating this in order to "turn it into syntax". The proposed syntax in #5571 seems to be mostly about currying, not piping. Currying is useful in lots of cases besides piping, so I don't think currying syntax should be tied to a |
Right now
Would be 3. I guess it would lower to
|
Here's a particularly horrible line from this PR:
Here's how it might be nicer chained:
|
Unless I'm mistaken, this would be might trickier to do with tight currying alone. |
That chained stuff is completely unreadable to me. |
I guess as someone who's been doing chaining in R for years the unchained alternative is completely unreadable to me. |
Both versions are difficult to grok. Better to write such expressions out more clearly if more verbosely. Best! |
Giving A terse anonymous-function syntax for more complicated cases, just to save writing |
I guess I'd be ok with tight currying too. Vanilla |> and chaining would look like this:
This is a bit of a mess. The convert line is the only one to take advantage of tight currying. I'm much more of fan of
ala |
I agree with @Sacha0 that this example is hideous as a single expression of any kind; it seems better to rewrite it entirely, breaking it into multiple lines or even multiple functions. |
I'm with you on that. Long chains are definitely confusing and breaking them into shorter chains is usually a good idea. |
@bramtayl, it's not so much the length of the chain in this case as all the conditionals. If these conditionals were split into appropriate functions (and probably handled by dispatch, not by runtime checks), it would be a lot more comprehensible (and also might help type inference): if elty1 == Int # probably better to handle this check via dispatch too
A = rand(1:7, n, n)
else
A = randn(n,n) |> complexify(elty1, _) |> (X -> X'X) |> chol |> fliptri(uplo1, _) |> Matrix{elty1}
end
t1(A) |
Yep that looks nice too. No argument from me about splitting out commonly used patterns into functions or that if/else sometimes looks a lot nicer than |
When multiple |
@@ -361,7 +361,6 @@ Julia applies the following order and associativity of operations, from highest | |||
| Bitshifts | `<< >> >>>` | Left | | |||
| Addition | `+ - \| ⊻` | Left[^2] | | |||
| Syntax | `: ..` | Left | | |||
| Syntax | `\|>` | Left | | |||
| Syntax | `<\|` | Right | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If |>
is being removed, shouldn't <|
also be removed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Not sure. Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. Yeah dunno.
I guess I've been convinced by @stevengj that |
In cases where a “tight” |
If that's the case, then why |
I 100% agree with that. that having terse lambdas is what the I'm not a huge fan of chaining these days. (It has its place sure)
I'm not sure what in particular you are thinking of.
Ergo this deprecation, right? |
I don't support this deprecation, because I think a I don't like the idea of a special currying syntax tied to |
|
Triage: instead of deprecating, we'll document that |
It might be nice to cherry-pick this PR, and keep a lot of the edits to remove |
No description provided.