Specify impl for Show[Symbol]
#4490
Conversation
On 2.12, `Symbol.toString` is e.g. `'foo`. On 2.13, it's `Symbol(foo)`. I want to use `Show` to get away from the vagaries of relying on `.toString` - to have a reliable nailed-down String representation for types. Want to use `Show` to build a pretty-printer, without having the output depend on which Scala version you're using. I tried overriding the `Show[Symbol]` instance locally, but I just got an ambiguous implicit error. Don't know how to not import this instance, since it's on the companion object for `Show`.
| @@ -25,5 +25,5 @@ import cats.Show | |||
|
|
|||
| trait SymbolInstances extends cats.kernel.instances.SymbolInstances { | |||
| implicit val catsStdShowForSymbol: Show[Symbol] = | |||
| Show.fromToString[Symbol] | |||
| Show.show("'" + _.name) | |||
There was a problem hiding this comment.
Or maybe this should just be .name - without the leading '? I think of Show as being something nice / sane for humans to read - we have .toString for diagnostic / debug output...
There was a problem hiding this comment.
I think this isn't safe since Symbol("foo bar") is allowed I think.
Instead I think we should check if the name is a valid Symbol literal (somehow, maybe regex?) and then we could use the '
There was a problem hiding this comment.
Not understanding what the issue is - what does being a valid 2.12 Symbol literal have to do with the Show representation? Is there a restriction that Show has to emit valid Scala source code? It already breaks that on the Show representation for String. 'foo bar is a valid string. That's the .toString of Symbol in Scala 2.12.
All this PR does is fix the Show implementation to the version already present on 2.12 - by inlining 2.12's definition of Symbol.toString: https://github.com/scala/scala/blob/2.12.x/src/library/scala/Symbol.scala#L30
There was a problem hiding this comment.
My concern is that imagine you interpolate this into a string, it would be ambiguous even to a human where the symbol ends.
As such I think it is a bad default.
Show is a lawless typeclass anyway, so I think there is an excellent argument it shouldn't exist in cats but maybe alleycats.
I'd rather see a typeclass more about encoding and decoding into strings. Like a pretty printer + parser. Such a typeclass can be lawful.
Here it's all bikeshed it seems. So I totally understand if you prefer a different bikeshed color.
There was a problem hiding this comment.
This is literally already the current Show[Symbol] impl on 2.12. Having different goals for Show seems outside the scope of this PR. I don't care what it is, so long as it's consistent. Do you have any alternate suggestions? 2.13's Symbol(foo)? Or no decoration, just foo?
On Haskell there's perhaps a convention of read . show = id, but I don't see either of those typeclasses mention that explicitly, Read is bad anyways, and there's no Read in Cats. I can't see any "rules" for what the expected Show output is - I gather it's just intended to give a string intended for human readability. I gather the convention is to roughly follow existing .toString representations. I see Kittens adds labels to case class parameters - still looks almost like Scala source. Are you proposing a novel string representation for Symbols, or just that Show shouldn't exist here in the first place, or...?
There are much better solutions if you expect machine-parsible output - the current Show impls break any hope of that all over the place already (e.g. strings are not quoted).
Redesigning the entire Show typeclass / documentation with the expectation of machine parseability isn't something I'm interested in (especially not for this PR).
I'm not proposing a different bikeshed color, I just want it to be the same across Scala versions.
Your concern about a human "not being able to tell where the symbol ends" already exists in Show and .toString for Symbol on 2.12, as well as Show[Symbol] for all Scala versions, etc. See the Show output at the end of the box on https://github.com/typelevel/kittens#derive-show.
That Show output is not valid Scala source code, nor is it intended to be. It's a human-readable representation of the value it was called on.
There was a problem hiding this comment.
As far as the Show instance for Symbol, would you prefer:
'foo bar(the current impl for 2.12)Symbol(foo bar)(the current impl for 2.13)- conditional depending on if it has a space, so sometimes
'fooand sometimesSymbol(foo bar)? - something else?
The only thing his PR is trying to do is to make that impl consistent across Scala versions. I'm not interested in redesigning / removing the Show typeclass on this PR.
This should be solvable, can you create a small example that demonstrates the problem? |
On 2.12,
Symbol.toStringis e.g.'foo. On 2.13, it'sSymbol(foo).I want to use
Showto get away from the vagaries of relying on.toString- to have a reliable nailed-down String representation for types.Want to use
Showto build a pretty-printer, without having the output depend on which Scala version you're using.I tried overriding the
Show[Symbol]instance locally, but I just got an ambiguous implicit error. Don't know how to not import this instance, since it's on the companion object forShow.