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

remove repr(::UUID), move UUID code #17026

Closed
ivarne opened this issue Jun 20, 2016 · 7 comments · Fixed by #23777
Closed

remove repr(::UUID), move UUID code #17026

ivarne opened this issue Jun 20, 2016 · 7 comments · Fixed by #23777
Assignees
Labels
help wanted Indicates that a maintainer wants help on an issue or pull request
Milestone

Comments

@ivarne
Copy link
Member

ivarne commented Jun 20, 2016

Looking at #17017, I noticed that there is room for more specialized (eg. faster) method specializations for repr().

julia> methods(repr)
#2 methods for generic function "repr"
repr(u::Base.Random.UUID) at random.jl:1252
repr(x) at strings/io.jl:86

The generic implementation in strings/io.jl:85 uses an IOBuffer, so it is probably much slower than it needs to be (especially for Strings which I think could be implemented as repr(s::string) = s). The documentation demands the result must be the same as showall, so testing should ensure that is still the case.

@ivarne ivarne added the good first issue Indicates a good issue for first-time contributors to Julia label Jun 20, 2016
@vtjnash
Copy link
Member

vtjnash commented Jun 20, 2016

Since this function is just an alias for showall, I propose we eliminate it.

@nalimilan
Copy link
Member

More precisely, it's an alias for sprint(showall, ...), right? I agree about deprecating it, since in 0.5 what's often needed is sprint(showcompact, ...) instead, and the fact that we have repr can cause confusion.

@ivarne ivarne added needs decision A decision on this change is needed and removed good first issue Indicates a good issue for first-time contributors to Julia labels Jun 20, 2016
@JeffBezanson
Copy link
Member

It is not the case that repr(x::String) = x, since repr adds quotes and escaping.

@ivarne
Copy link
Member Author

ivarne commented Jun 21, 2016

@JeffBezanson Thanks for noticing. I somehow managed to convince myself that it did not, but my REPL history shows I did not actually try.

@StefanKarpinski StefanKarpinski added this to the 0.6.0 milestone Sep 13, 2016
@JeffBezanson
Copy link
Member

If anything, the repr method for UUID should be removed since nothing else defines it directly. Instead it should define string or show. Then the UUID code is a good candidate for #5155, since AFAICT it's not used anywhere in Base.

@JeffBezanson JeffBezanson changed the title Specialized repr methods remove repr(::UUID), move UUID code Oct 17, 2016
@tkelman
Copy link
Contributor

tkelman commented Jan 5, 2017

Some of the UUID code will be needed for Pkg3, but if that itself is a package then it can have other-package dependencies that aren't all Base code.

@tkelman tkelman modified the milestones: 1.0, 0.6.0 Jan 5, 2017
@StefanKarpinski
Copy link
Member

Making this a method of string would be the way to go here. Moving UUIDs out would be good too.

@Keno Keno added help wanted Indicates that a maintainer wants help on an issue or pull request and removed needs decision A decision on this change is needed labels Jul 20, 2017
@StefanKarpinski StefanKarpinski added the needs decision A decision on this change is needed label Jul 20, 2017
@Keno Keno removed the needs decision A decision on this change is needed label Jul 20, 2017
@vtjnash vtjnash self-assigned this Sep 5, 2017
vtjnash added a commit that referenced this issue Sep 19, 2017
This is the typical place for such a method.

fix #17026
vtjnash added a commit that referenced this issue Sep 20, 2017
This is the typical place for such a method.

fix #17026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Indicates that a maintainer wants help on an issue or pull request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants