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

Add pretty printing for splat(f) #42717

Merged
merged 11 commits into from
May 9, 2022
Merged

Conversation

Seelengrab
Copy link
Contributor

When debugging calls with different invocations of splat, this helps to identify which function was being splatted into.

master:

julia> Base.splat(cos)
#92 (generic function with 1 method)

julia> f = Base.splat(sin)
#92 (generic function with 1 method)

julia> f(1)
0.8414709848078965

julia> [f]
1-element Vector{Base.var"#92#93"{typeof(sin)}}:
 #92 (generic function with 1 method)

julia> using Logging

julia> @info f
[ Info: #92

This PR:

julia> Base.splat(cos)
splat(cos)

julia> f = Base.splat(sin)
splat(sin)

julia> f(1)
0.8414709848078965

julia> [f]
1-element Vector{Base.Splat{typeof(sin)}}:
 splat(sin)

julia> @info f
[ Info: splat(sin)

Should splat be exported? It's pretty useful.

@Seelengrab
Copy link
Contributor Author

Failures are Distributed related (EAGAIN: Resource temporarily not available), seems like some tasks can't spawn their setenv(...) invocations. Unrelated to this PR.

@mbauman mbauman added the display and printing Aesthetics and correctness of printed representations of objects. label Oct 20, 2021
@mbauman
Copy link
Member

mbauman commented Oct 20, 2021

What if we printed it like x -> sin(x...)? That way it'd tell you what it did? Or is that too fancy?

@Seelengrab
Copy link
Contributor Author

Seelengrab commented Oct 20, 2021

Well it'd be lying to you, as that's not how its implemented (edit: in this PR) 🤷 That used to be the old implementation though. Changing it was necessary to allow for easier printing.

Compared to the old way of just printing whatever number anonymous function the result of splat happened to be, this representation is already a big step up and allows you to do ?Base.splat(sin) (we'd have to export it to make ?splat work) and you'll get the docs for it, where it says that this is equivalent to the lambda version.

Since it's not exported, I don't think it's used very wildly right now. I'm working on a package (PropCheck.jl) where using splat to interact with it makes everything a bunch easier (see e.g. the tests in BigNums.jl). I got annoyed with not knowing which function was wrapped, hence this PR.

@Seelengrab
Copy link
Contributor Author

I'm talking about this PR 😅 In this PR, this is no longer a simple anonymous function, precisely because it makes printing easier. If it were not for the new struct, I'd have to call splat first, to get the anonymous function to customize its printing.

@Seelengrab
Copy link
Contributor Author

I should note that this works the exact same way as the anonymous function, except doing the hoisting to a struct manually instead of implicitly by the compiler.

base/operators.jl Outdated Show resolved Hide resolved
base/operators.jl Outdated Show resolved Hide resolved
@vtjnash
Copy link
Member

vtjnash commented Oct 24, 2021

string can be overloaded, though it is uncommon, and usually just for performance to give the same result

@Seelengrab
Copy link
Contributor Author

I think you meant to answer in the review thread above ;)

@Seelengrab
Copy link
Contributor Author

I've incorporated the feedback, fixed printing, added the deprecation and replaced uses of the old syntax in the rest of Base. It's also rebased to the current master. Now let's hope nothing broke 😬

@Seelengrab
Copy link
Contributor Author

Seelengrab commented Mar 9, 2022

Rebase & rerun of CI - I've also moved the deprecation notice to the 1.9 deprecation block, since this definitely isn't going to make 1.8 anymore. I'll write NEWS.md once CI is happy.

@vtjnash
Copy link
Member

vtjnash commented Mar 10, 2022

Add export?

@Seelengrab
Copy link
Contributor Author

$ /etc/buildkite-agent/plugins/github-com-JuliaCI-external-buildkite-buildkite-plugin-v1/hooks/post-checkout
--
  | fatal: destination path '.buildkite' already exists and is not an empty directory.
  | 🚨 Error: The plugin external-buildkite post-checkout hook exited with status 128

Uuh.. Unrelated? Doesn't look good though..


In either case, I've also added a NEWS.md entry since CI was green last night. Should be good to go now.

@Seelengrab
Copy link
Contributor Author

The macos failure seems to stem from Libgit2:

Test Failed at /Users/julia/buildbot/worker-tabularasa/tester_macos64/build/share/julia/stdlib/v1.9/LibGit2/test/libgit2.jl:1754
Expression: LibGit2.headname(repo) == master_branch
Evaluated: "main" == "master"

@Seelengrab
Copy link
Contributor Author

I don't know where this should fall in the release cycle, but it'd be nice to get this in a future 1.9 :)

@Seelengrab
Copy link
Contributor Author

Rebase on master!

@Seelengrab
Copy link
Contributor Author

Failure in BuildKite macOS is cmdlineargs, seems unrelated.

@Seelengrab Seelengrab requested a review from vtjnash April 24, 2022 08:35
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Apr 24, 2022
@vtjnash vtjnash merged commit ef10e52 into JuliaLang:master May 9, 2022
@Seelengrab Seelengrab deleted the splat_printing branch May 9, 2022 15:47
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label May 13, 2022
@knuesel knuesel mentioned this pull request Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants