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

improve REPL printing of sets #33300

Merged
merged 2 commits into from
Oct 30, 2019
Merged

improve REPL printing of sets #33300

merged 2 commits into from
Oct 30, 2019

Conversation

rfourquet
Copy link
Member

@rfourquet rfourquet commented Sep 17, 2019

Compared to arrays and dicts, the default REPL printing of sets is less than ideal for inspecting values.
Master:

julia> Set(rand(100))
Set([0.9355287178152043, 0.4928021501163431, 0.30965545692897645, 0.12977832204668327, 0.21199393799696042, 0.0049669103311482665, 0.21544680124723192, 0.94211
31090732746, 0.1757484374656193, 0.9827906624667768    0.9395587064783664, 0.6661801938225571, 0.6137788625422396, 0.8401334617771041, 0.1553607216380093, 0.8
201636488737938, 0.7213369975241664, 0.946783923950687, 0.9626916317380967, 0.9239185348013443])

PR:

julia> Set(rand(100))
Set{Float64} with 100 entries:
  0.2570901274036239
  0.07658455754806237
  0.16876087627652803
  0.6875079709294418
  0.4490615526957693
  0.9858242705954456
  0.24446960302165
  0.7804304622569547
  0.3584736990667057
  0.10787858192490529
  0.01588708187652732
  0.6316046571808667
  0.45151298050579336
  0.3619189322408094
  0.2619333768991221
  

@rfourquet rfourquet added collections Data structures holding multiple items, e.g. sets display and printing Aesthetics and correctness of printed representations of objects. needs tests Unit tests are required for this change labels Sep 17, 2019
@KristofferC
Copy link
Member

It is a bit different from how Array prints the , cf:

julia> a = rand(100)
100-element Array{Float64,1}:
 0.4746815092073091
 0.9484816361221338
 0.613811448828379
 0.5142341357868621
 0.05873629586930762
 0.702505071916633
 0.45982443303453424
 ⋮
 0.30567035367024586
 0.06633560832376939
 0.7123200471052471
 0.4799413972061428
 0.49309094787857055
 0.5925469710385052
 0.28220057407708987

Not sure if it matters.

@rfourquet
Copy link
Member Author

rfourquet commented Sep 17, 2019

It is a bit different from how Array prints the ⋮

It's on purpose, there is no particular ordering for sets, so printing the "last" elements (skippling middle ones) is not specially meaningfull compared to just printing elements until there is no more space. Note that this is how dicts are shown at the REPL (ellipsis at the end). Also, I plan to propose changing the show(::Set) method to put the ellipsis at the end.

@KristofferC
Copy link
Member

Makes sense.

base/show.jl Outdated
function summary(io::IO, t::AbstractSet)
n = length(t)
showarg(io, t, true)
print(io, " with ", n, (n==1 ? " entry" : " entries"))
Copy link
Member

Choose a reason for hiding this comment

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

I know dicts use "entries" but I think "elements" makes more sense for sets.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I copy-pasted from dicts' method without thinking twice.

@rfourquet
Copy link
Member Author

Given the support received by this PR, I went ahead and fixed test failures. I think this would be good to go on my side if CI turns green.

@@ -452,9 +452,6 @@ let a = Expr(:quote,Expr(:$,:x8d003))
@test eval(eval(Meta.parse(repr(a)))) == 2
end

# issue #9865
Copy link
Member Author

Choose a reason for hiding this comment

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

This issue is now tested in other places of the code.

@StefanKarpinski StefanKarpinski added needs news A NEWS entry is required for this change and removed needs tests Unit tests are required for this change labels Oct 1, 2019
@StefanKarpinski
Copy link
Member

I like it and am tempted to merge but could you just add a NEWS note about the change?

@mbauman mbauman removed the needs news A NEWS entry is required for this change label Oct 4, 2019
@rfourquet
Copy link
Member Author

Good to go? triage?

@StefanKarpinski StefanKarpinski merged commit 306182f into master Oct 30, 2019
@StefanKarpinski StefanKarpinski deleted the rf/set-display branch October 30, 2019 15:45
@rfourquet
Copy link
Member Author

Oh I had not noticed you merged it, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants