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

AnnotatedStrings are not repr-eval-able. #54193

Closed
LilithHafner opened this issue Apr 22, 2024 · 6 comments · Fixed by #54308
Closed

AnnotatedStrings are not repr-eval-able. #54193

LilithHafner opened this issue Apr 22, 2024 · 6 comments · Fixed by #54308
Labels
strings "Strings!"

Comments

@LilithHafner
Copy link
Member

eval(Meta.parse(repr(x))) == x should hold whenever possible. It does not hold for AnnotatedStrings.

An example of a symptom of this problem that came up when testing #53715 is that @test equality hints are misleading for AnotatedStrings with annotation mismatches:

Screenshot of MWE with color

julia> using StyledStrings, Test

julia> x = styled"{red:abc}"
"abc"

julia> @test x == "abc"
Test Failed at REPL[4]:1
  Expression: x == "abc"
   Evaluated: "abc" == "abc"

ERROR: There was an error during testing

julia> repr(x)
"\"abc\""

Adding a special case that notes "annotation mismatch" would be an improvement, but I'm guessing repr is the root cause.

cc @tecosaur

@tecosaur
Copy link
Contributor

tecosaur commented Apr 23, 2024

I think the thing that makes repr a bit tricky (in general) is that it invokes show(::IO, x), and because of how show(::IO, x) is used in certain "display this value" situations, it could make some displays rather ugly. Off-hand I can't recall examples of this, but if needed I'm sure we can find a few of these cases.

Perhaps such uses of the 2-arg show should be changed to 3-arg show and the 2-arg AnnotatedString show should be removed? I'm not sure.

NB: Ptr is also like this, eval(Meta.parse(repr(ptr))) != ptr.

@tecosaur
Copy link
Contributor

Ok, one path forwards is to implement a specialised show method, but there does seem to be a tension between the "2-arg show is the Julia-eval version" and "2-arg show is the more concise version" aspects in the documentation of show.

I'd be keen to hear thoughts on how to best resolve this.

@tecosaur tecosaur added the strings "Strings!" label Apr 29, 2024
@LilithHafner
Copy link
Member Author

I don't see "2-arg show is the more concise version" in the docs. That happens to be the case because parseable Julia code is often more concise than the human-readable version (e.g. π vs π = 3.1415926535897...) but is not the case when the parseable Julia code is less concise (e.g. DateTime("2024-04-29T16:36:16.735") vs 2024-04-29T16:36:16.735)

I think this would be reasonable, and an improvement over showing "abc" in both cases:

julia> show(stdout, Day(1))
Day(1)
julia> show(stdout, MIME("text/plain"), Day(1))
1 day
julia> show(stdout, Base.AnnotatedString("abc", [(1:2, :value => π)]))
Base.AnnotatedString("abc", [(1:2, :value => π)])
julia> show(stdout, MIME("text/plain"), Base.AnnotatedString("abc", [(1:2, :value => π)]))
"abc"

@tecosaur
Copy link
Contributor

I took that implication from

For a more verbose human-readable text output for objects of type T, define show(io::IO, ::MIME"text/plain", ::T)

but now I've got the second opinion I've asked for, I'll happily PR

function show(io::IO, astr::A) where {A <: AnnotatedString}
    show(io, A)
    print(io, '(')
    show(io, astr.string)
    print(io, ", ")
    show(io, annotations(astr))
    print(io, ')')
end

🙂

@LilithHafner
Copy link
Member Author

Ah, I missed that word. But yes, I still think that reprable is more important than concise in this case.

@tecosaur
Copy link
Contributor

Cool, I'll go ahead with the PR then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
strings "Strings!"
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants