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.

Sign up for GitHub

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

Jump to bottom

Specify impl for Show[Symbol] #4490

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Specify impl for Show[Symbol] #4490

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/scala/cats/instances/SymbolInstances.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 '

Copy link
Author

@LeifW LeifW Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@LeifW LeifW Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@LeifW LeifW Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 'foo and sometimes Symbol(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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related (there's no Read typeclass - Show is for human-readable output, not intended to be machine-parseable): #932 (comment)

}