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

REPL: improve handling of non-printable characters (to prevent messing up terminal) #9451

Merged
merged 1 commit into from Feb 3, 2021

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Jan 23, 2021

So colors can come through unscathed.

Fixes scala/bug#12276

cf aafcc10

REPL strips its wrapper identifiers from output, and also tries to suppress output that might mess up the terminal.

This commit simplifies the heuristic on "what to pass" to a regex for the alternatives: control sequences for color output, non-printable chars which are replaced by ?, and the existing pattern for wrapper names.

@scala-jenkins scala-jenkins added this to the 2.13.5 milestone Jan 23, 2021
@som-snytt

This comment has been minimized.

@lrytz lrytz requested a review from SethTisue January 25, 2021 09:11
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

LGTM on paper, but it's not easy to follow the change. Could you outline/describe the change some more, please?

test/files/run/t12276.check Outdated Show resolved Hide resolved
@som-snytt
Copy link
Contributor Author

som-snytt commented Jan 25, 2021

The update takes Dale's suggestion to show test output per line, which is not as pretty, but is much easier to scan. The weird aspect is that the line endings are implicit and not shown in the hex dump. NBD.

Improved comments about the pattern.

Also push the alternative ILoop.runForTranscript into ReplTest, so the test class only supplies a flag, similar to existing inSession flag. These represent config external to settings. It would be nicer to supply a ShellConfig directly: trait ILoop.TestConfig would delegate, and the test class could override instead of passing arguments. (Edit: made that change.)

@SethTisue SethTisue added the tool:REPL Changes in the Scala REPL Interpreter label Jan 26, 2021
@SethTisue SethTisue changed the title Chill out repl ctrl-char filter. REPL: improve handling of non-printable characters (to prevent messing up terminal) Jan 26, 2021
@SethTisue
Copy link
Member

@som-snytt mind squashing into fewer commits? then let's :shipit:

@SethTisue
Copy link
Member

@som-snytt ping. (if you're busy this week, let me know and I'll take care of it.)

@som-snytt

This comment has been minimized.

@dwijnand dwijnand merged commit 9813be7 into scala:2.13.x Feb 3, 2021
@som-snytt som-snytt deleted the issue/12276 branch February 3, 2021 15:49
@lrytz
Copy link
Member

lrytz commented Mar 8, 2021

This seems a bit too eager...

2.13.4:

scala> "\uCAFE caffè"
val res0: String = 쫾 caffè

2.13.5

scala> "\uCAFE caffè"
val res0: String = ? caff?

@som-snytt
Copy link
Contributor Author

US-ASCII FTW. "POSIX character classes (US-ASCII only)".

// group 1 is the CSI command letter, where 'm' is color rendition
// group 2 is a sequence of chars to be rendered as `?`: anything non-printable and not some space char
// additional groups are introduced by linePattern but not used
private lazy val cleaner = raw"$csi|([^\p{Print}\p{Space}]+)|$linePattern".r
Copy link
Contributor Author

Choose a reason for hiding this comment

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

([\p{Cntrl}&&[^\p{Space}]]+) for group 2

Copy link
Member

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tool:REPL Changes in the Scala REPL Interpreter
Projects
None yet
5 participants