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

Printing StackOverflowError #29

Open
keorn opened this issue Dec 3, 2021 · 7 comments
Open

Printing StackOverflowError #29

keorn opened this issue Dec 3, 2021 · 7 comments

Comments

@keorn
Copy link

keorn commented Dec 3, 2021

If one implements custom AbstractMeasure and not override any printing methods this method leads to an error:
https://github.com/cscherrer/MeasureBase.jl/blob/master/src/utils.jl#L3

That is most likely because pprint falls back to show at some point.

Replicate in REPL:

julia> using MeasureBase: AbstractMeasure
julia> struct A <: AbstractMeasure end
julia> A()
Error showing value of type A:
ERROR: StackOverflowError:

Few ways to address it, not sure which one you prefer.

@cscherrer
Copy link
Collaborator

Thanks @keorn . I didn't realize this was a problem in master, but I did hit this a couple of times in the refactoring I'm doing in dev. I added this, which I think gives a reasonable fallback:

function Pretty.tile(d::AbstractMeasure)
    M = Symbol(constructor(d))
    the_names = fieldnames(typeof(d))
    isempty(the_names) && return Pretty.literal(M)*Pretty.literal("()")
    Pretty.list_layout(Pretty.tile.([getfield(d, n) for n in the_names]); prefix=M)
end

Then we can override that for particular types whenever it makes sense.

@keorn
Copy link
Author

keorn commented Dec 3, 2021

I would prefer if by default AbstractMeasure would print in the default Base way. Just because something is an AbstractMeasure should not affect its string representation.

@cscherrer
Copy link
Collaborator

Hmm good point, that is a little strange. I mean, it's usually what I want, but kind of arbitrary to impose on new types an end-user might build.

@cscherrer
Copy link
Collaborator

@cscherrer
Copy link
Collaborator

I've updated this so the default mimics Base as closely as possible:

function Pretty.tile(d::M) where {M<:AbstractMeasure}
    the_names = fieldnames(typeof(d))
    result = Pretty.literal(repr(M))
    isempty(the_names) && return result * Pretty.literal("()")
    Pretty.list_layout(Pretty.tile.([getfield(d, n) for n in the_names]); prefix=result)
end

I'll close this for now, but I'm open to alternatives if the PrettyPrinting issue leads to a better approach or if you have a PR. Please reopen in that case.

@keorn
Copy link
Author

keorn commented Dec 5, 2021

Great, thanks. This temporary solution makes sense.

@cscherrer cscherrer reopened this Dec 6, 2021
@cscherrer
Copy link
Collaborator

Reopening, since @xitology gave some helpful information in response to MechanicalRabbit/PrettyPrinting.jl#5. We don't need to finalize this right away, but my quick fix shouldn't be considered the final solution.

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

No branches or pull requests

2 participants