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

Consider deleting custom show methods for types #321

Closed
andreasnoack opened this issue Apr 22, 2020 · 5 comments · Fixed by #322
Closed

Consider deleting custom show methods for types #321

andreasnoack opened this issue Apr 22, 2020 · 5 comments · Fixed by #322

Comments

@andreasnoack
Copy link
Contributor

I.e.

function show(io::IO, x::Type{T}) where T<:Quantity
invoke(show, Tuple{IO, typeof(x)}, IOContext(io, :showoperators=>true), x)
end
function show(io::IO, x::Type{T}) where T<:Unitlike
invoke(show, Tuple{IO, typeof(x)}, IOContext(io, :showoperators=>true), x)
end
. There are two reasons for this.

The first is that, according to the compiler team, the base methods are not supposed to be overloaded. It's not documented and not obvious but it's been assumed that show(::IO, ::Type) isn't overloaded by user code so doing that can lead to very odd issues. E.g. the definitions in this package might have caused Documenter to hang for one of my packages because Documenter prints the signature of methods with missing docs.

The second reason is that it can be super confusing when types print differently from what they really are. I've had some issues with that when using this package because I based some function signatures on how the types were printed in the REPL only to get an error because the signature didn't match.

@cstjean
Copy link
Contributor

cstjean commented Apr 22, 2020

The first is that, according to the compiler team, the base methods are not supposed to be overloaded. It's not documented and not obvious but it's been assumed that show(::IO, ::Type) isn't overloaded by user code so doing that can lead to very odd issues

The docs really need to change then, because this section clearly overloads Base.show(io::IO, z::Polar):

https://docs.julialang.org/en/v1.4/manual/types/#man-custom-pretty-printing-1

@andreasnoack
Copy link
Contributor Author

Defining Base.show(io::IO, z::Polar) is fine. The problem would be to define e.g. Base.show(io::IO, z::Type{<:Polar})

@cstjean
Copy link
Contributor

cstjean commented Apr 22, 2020

Oh, right, sorry for the noise. I need to read more carefully.

@timholy
Copy link
Contributor

timholy commented Apr 22, 2020

Old history here: JuliaLang/julia#13306. Not sure how much of a modern problem it is, but I've been paranoid about specializing show for types ever since.

I wonder if we need showtype to do pretty-printing of types and decide on which places use the pretty and which use the raw. In general I would have thought method signatures should be printed prettily, but it sounds like that would have caused the same bug you got with Documenter.

@ajkeller34
Copy link
Collaborator

It would be very useful to have something like showtype. Unitful uses values as type parameters (dimensions, for example). I don't think it is clear how to make such types show in a way where you can paste them into a REPL and have them evaluate correctly, in general. The values don't know they are being shown inside of a type signature.

timholy added a commit to JuliaGeometry/GeometryBasics.jl that referenced this issue Jul 1, 2020
Overloading `show` for types is strongly discouraged. I'm currently tracking down a Julia segfault that's caused by this line.

Refs:
- JuliaMath/FixedPointNumbers.jl#11
- PainterQubits/Unitful.jl#321
briochemc added a commit to briochemc/Unitful.jl that referenced this issue Aug 10, 2020
* upstream/master: (22 commits)
  Fix printing of complex and logarithmic quantities (PainterQubits#366)
  Remove @_doctables (PainterQubits#363)
  Fix a doctest (PainterQubits#365)
  Release v1.3.0
  Add page headers to documentation (PainterQubits#361)
  Fix link to documentation in README.md: latest -> dev. (PainterQubits#360)
  Properly document Quantity constructor (PainterQubits#357)
  Delete show(::IO, ::Type) methods. Closes PainterQubits#321 (PainterQubits#322)
  Throw error for div etc. with affine quantities (PainterQubits#354)
  Fixes for the documentation (PainterQubits#356)
  Enable Travis on macOS (PainterQubits#355)
  Switch to a more standard Documenter setup (PainterQubits#353)
  Update repo URL (PainterQubits#352)
  Delete duplicate @affineunit docstring in the documentation (PainterQubits#349)
  Fix some doctests (PainterQubits#345)
  Add AbstractQuantity docstring to docs (PainterQubits#351)
  Provide Base.isless() for LogScaled values (PainterQubits#315)
  Small fixes to documentation (PainterQubits#341)
  Add CompatHelper, reduce frequency of TagBot to once per day (PainterQubits#337)
  Fix conversion of fractional units (PainterQubits#335)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants