Skip to content

[SUPERSEDED] Use ellipsis instead of silent truncation in stringOf #10178

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
Closed
wants to merge 1 commit into from
Closed

[SUPERSEDED] Use ellipsis instead of silent truncation in stringOf #10178

wants to merge 1 commit into from

Conversation

som-snytt
Copy link
Contributor

Fixes scala/bug#12337

➜  ~ scala -Dscala.repl.maxprintstring=50
Welcome to Scala 2.13.10-20221005-211743-e04f32b (OpenJDK 64-Bit Server VM, Java 19).
Type in expressions for evaluation. Or try :help.

scala> (1 to 1000).map(i => "X"*5).toList
val res0: List[String] = List(XXXXX, XXXXX, XXXXX, XXXXX, XXXXX, ...)

scala> (1 to 1000).map(i => "X"*50).toList
val res1: List[String] = List(XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX...)

scala> "List(XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX...)".length
val res2: Int = 50

scala> "X"*50
val res3: String = XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX

scala> "X"*51
val res4: String = XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX...

@scala-jenkins scala-jenkins added this to the 2.13.11 milestone Oct 5, 2022
@som-snytt som-snytt mentioned this pull request Oct 5, 2022
Closed
@som-snytt som-snytt marked this pull request as ready for review October 5, 2022 23:37
@som-snytt som-snytt requested a review from SethTisue October 5, 2022 23:42
@som-snytt som-snytt mentioned this pull request Oct 6, 2022
Open
@som-snytt
Copy link
Contributor Author

@SethTisue it's so pretty. I wonder if you'll feel sorry if 2.13.10 doesn't have it. I mean the rueful thoughts that occur to you years later while tarrying in a coffee shop during an unexpected rain storm.

lrytz
lrytz reviewed Oct 7, 2022
View reviewed changes
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

The new pretty version is not yet in use, IIUC?

src/library/scala/runtime/ScalaRunTime.scala
Comment on lines +382 to +385
if (x.productArity == 1 && !x.productElement(0).isInstanceOf[Boolean])
s"${x.productPrefix}(${inner(x.productElement(0))})"
else
_toString1(x, inner(_))
Copy link
Member

Choose a reason for hiding this comment

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

_toString0 is unused, and this looks almost like it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not unused forever, or at least by me in my REPL.

src/library/scala/runtime/ScalaRunTime.scala
* @param arg the value to stringify
* @param maxLength limit the length of the result string
* @param verboseProduct extra user-friendly
* @return a string representation of arg.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Nit, but the Scaladoc should be on the 3-param overload

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that, but my thought was, scaladoc should be smart enough to figure out overloads. I just drove someone to the airport, and my ordinary Honda knows how to stay inside the white lines!

@SethTisue SethTisue modified the milestones: 2.13.11, 2.13.10 Oct 7, 2022
@SethTisue SethTisue mentioned this pull request Oct 18, 2022
Closed
@SethTisue
Copy link
Member

I mean the rueful thoughts that occur to you years later while tarrying in a coffee shop during an unexpected rain storm.

Those thoughts take on extra existential piquancy when they occur during my travels in Europe. 🧑‍🎨🪗

An ellipsis does seem strongly preferable to silent truncation, so presumably we're going to merge this, but Lukas's nits seem valid, and I'd also like my memory refreshed about:

The new pretty version is not yet in use, IIUC?

@lrytz
Copy link
Member

lrytz commented Oct 27, 2022

I think we can close this in favor of #10180, where the nit is also addressed

@lrytz lrytz closed this Oct 27, 2022
@som-snytt
Copy link
Contributor Author

I need to add a comment on the (unused) API on the other PR.

@SethTisue SethTisue removed this from the 2.13.11 milestone Oct 27, 2022
@som-snytt som-snytt changed the title Use ellipsis instead of silent truncation in stringOf [SUPERSEDED] Use ellipsis instead of silent truncation in stringOf Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lrytz lrytz lrytz left review comments

@SethTisue SethTisue Awaiting requested review from SethTisue

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

REPL element truncation should not silently truncate
4 participants
@som-snytt @SethTisue @lrytz @scala-jenkins