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

[ZEPPELIN-3988] Paragraph Text output includes \r\n is not displayed correctly. #3302

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
3 participants
@Leemoonsoo
Copy link
Member

Leemoonsoo commented Feb 4, 2019

What is this PR for?

When paragraph text output includes \r\n, it is not displayed correctly in paragraph result.

Expected result
image

Current behavior (displays empty result)
image

What type of PR is it?

Bug Fix

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-3988

How should this be tested?

run

%python
print("Hello world\r\n")

and see if output is displayed correctly.

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no
return newGenerated.slice(0, -1);
} else {
return str;
}

This comment has been minimized.

Copy link
@Leemoonsoo

Leemoonsoo Feb 4, 2019

Author Member

Reason implementation of checkAndReplaceCarriageReturn moved to new class is for testability.
Controller method is hard to test. Unless method is referenced by $scope


it('scope should be initialized', function() {
expect(scope).toBeDefined();
expect(controller).toBeDefined();

This comment has been minimized.

Copy link
@Leemoonsoo

Leemoonsoo Feb 4, 2019

Author Member

Does not have meaningful test now. But later, test for result.controller.js can be easily added when necessary.
Note that controller method, such as const checkAndReplaceCarriageReturn = ... is not accessible here. Unlike scope is accessible and testable here.

for (let str of strArr) {
if (/\r/.test(str)) {
let splitCR = str.split('\r');
newGenerated += splitCR[splitCR.length - 1] + '\n';

This comment has been minimized.

Copy link
@Leemoonsoo

Leemoonsoo Feb 4, 2019

Author Member

Looks like this line cause problem

}

checkAndReplaceCarriageReturn() {
return this.data.replace(/(\r\n)/gm, '\n');

This comment has been minimized.

Copy link
@Leemoonsoo

Leemoonsoo Feb 4, 2019

Author Member

@zjffdu I checked #2729. And i'm not sure just replacing \r\n to \n will be okay or not. Could you review?

This comment has been minimized.

Copy link
@felixcheung

felixcheung Feb 4, 2019

Member

I think the case was that it could have \r by itself..

@felixcheung
Copy link
Member

felixcheung left a comment

does seem related to #2729

@zjffdu

This comment has been minimized.

Copy link
Contributor

zjffdu commented Feb 11, 2019

@Leemoonsoo I tested this PR, but unfortunately it break the function of progress bar that #2729 implements. Progress bar is very important in machine learning libraries, specially for deep learning libraries, such as tensorflow, keras. Is it possible to keep the progress bar behavior ? You can use the following code snippet to test the progress bar.

import time,sys
end_val = 10
bar_length = 20
for i in xrange(0, end_val + 1):
    time.sleep(0.5)
    percent = float(i) / end_val
    hashes = '#' * int(round(percent * bar_length))
    spaces = ' ' * (bar_length - len(hashes))
    sys.stdout.write("\rPercent: [{0}] {1}%".format(hashes + spaces, int(round(percent * 100))))
    #print "Percent: [{0}] {1}%".format(hashes + spaces, int(round(percent * 100)))
@Leemoonsoo

This comment has been minimized.

Copy link
Member Author

Leemoonsoo commented Feb 11, 2019

Thanks for explaining. I'll add more commits to support progress bar behavior.

@Leemoonsoo

This comment has been minimized.

Copy link
Member Author

Leemoonsoo commented Feb 12, 2019

@zjffdu @felixcheung I pushed new commit. Now it handles \n, \r and \r\n correctly. (current master can't display output of %python !ls -al while it new line \r\n)

image

@Leemoonsoo Leemoonsoo closed this Feb 12, 2019

@Leemoonsoo Leemoonsoo reopened this Feb 12, 2019

@zjffdu

This comment has been minimized.

Copy link
Contributor

zjffdu commented Feb 13, 2019

Thanks @Leemoonsoo I tested locally, it works well

@Leemoonsoo

This comment has been minimized.

Copy link
Member Author

Leemoonsoo commented Mar 1, 2019

Merge to master and branch-0.8

@asfgit asfgit closed this in d07ae2c Mar 1, 2019

asfgit pushed a commit that referenced this pull request Mar 1, 2019

[ZEPPELIN-3988] Paragraph Text output includes \r\n is not displayed …
…correctly.

### What is this PR for?
When paragraph text output includes `\r\n`, it is not displayed correctly in paragraph result.

Expected result
![image](https://user-images.githubusercontent.com/1540981/52189818-da9e8680-27ef-11e9-8d17-790101677a9b.png)

Current behavior (displays empty result)
![image](https://user-images.githubusercontent.com/1540981/52189831-e8540c00-27ef-11e9-8f35-bc79b21669e3.png)

### What type of PR is it?
Bug Fix

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-3988

### How should this be tested?
run

```
%python
print("Hello world\r\n")
```

and see if output is displayed correctly.

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no

Author: Lee moon soo <moon@apache.org>

Closes #3302 from Leemoonsoo/ZEPPELIN-3988 and squashes the following commits:

b3f0290 [Lee moon soo] bring back carriage return routine
06c5d40 [Lee moon soo] improve checkAndReplaceCarriageReturn and move it to a separate class to add tests

(cherry picked from commit d07ae2c)
Signed-off-by: Lee moon soo <moon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.