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

Replace ansi_up with code from Classic Notebook #5336

Merged
merged 8 commits into from Oct 18, 2018

Conversation

@mgeier
Copy link
Contributor

@mgeier mgeier commented Sep 18, 2018

Fixes #3773.

I simply removed everything that is called "ansi_up" or "AnsiUp" and copy-pasted the JavaScript code from the Classic Notebook.

  • HTML escaping is still missing because I don't know which library is available to do that.

  • I don't know if carriage-return-escaping-handling is necessary or not.

I have no clue about TypeScript, so I just used the old JavaScript and sprinkled a few types around in order to silence all compiler errors.

  • I made some minor changes to satisfy the pre-commit linter, but there were two complaints left:
ERROR: packages/rendermime/src/renderers.ts[655, 11]: Assignments in conditional expressions are forbidden
ERROR: packages/rendermime/src/renderers.ts[663, 24]: Missing radix parameter

I don't know what's the best alternative for the first one and I have no idea what the second one means.

  • The functions should probably be renamed and moved somewhere else and there are very likely many stylistic errors.

... but it somehow compiles and runs!

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 18, 2018

Thanks for taking this on! I fixed the lint errors and added lodash.escape.

@mgeier
Copy link
Contributor Author

@mgeier mgeier commented Sep 18, 2018

Thanks @blink1073!

I've updated the tests to reflect the changed CSS classes.

There is a CI failure that mentions "ansi_up": https://travis-ci.org/jupyterlab/jupyterlab/jobs/430076153
I thought I've removed all occurrences of that ...?

The other CI failure are probably unrelated to this PR?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 19, 2018

I reverted the changes to jupyterlab/staging, which only work with released versions of our packages. The docs failure is known and will be resolved when Sphinx 1.8.1 is released.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 19, 2018

Is there carriage return escaping in the classic notebook?

@mgeier
Copy link
Contributor Author

@mgeier mgeier commented Sep 19, 2018

Is there carriage return escaping in the classic notebook?

Sorry, I misspoke. I didn't mean "escaping" but rather something like "handling".

There are some related JS functions there: https://github.com/jupyter/notebook/blob/master/notebook/static/base/js/utils.js#L477-L504

I didn't look into how JupyterLab handles carriage returns.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 19, 2018

Ah, we handle that in the cell output model here.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 19, 2018

I think if your rename ansispan -> ansiSpan and move the new code under Private (add export to fixConsole and call as Private.fixConsole), this is good to go pending a review from @ellisonbg on the UX.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 19, 2018

@blink1073 blink1073 added this to the 1.0 milestone Sep 19, 2018
@mgeier
Copy link
Contributor Author

@mgeier mgeier commented Sep 19, 2018

@blink1073 Thanks for your help! I've moved the code to the Private namespace and have removed the leading underscores. I've also added some rudimentary documentation to all functions.

I didn't really like the name fixConsole(), so I incorporated the HTML escaping into ansiSpan() which is now called as Private.ansiSpan().

Please let me know if I should change anything else.

@mgeier mgeier changed the title WIP: Try to replace ansi_up with code from Classic Notebook Replace ansi_up with code from Classic Notebook Sep 19, 2018
@mgeier
Copy link
Contributor Author

@mgeier mgeier commented Sep 19, 2018

As a bonus commit I've added cb9e894, which re-introduces some ES6 constructs which I had to remove in jupyter/notebook@245287a.

Feel free to drop this commit if you don't like it.

@mgeier
Copy link
Contributor Author

@mgeier mgeier commented Sep 19, 2018

I was wondering if I should add return type annotations. If yes, here you go: 416ae66.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 20, 2018

...and just like that you're a TypeScript developer 😉. Code changes LGTM, thanks!

@jasongrout jasongrout requested a review from tgeorgeux Sep 26, 2018
@blink1073 blink1073 removed this from the 1.0 milestone Sep 28, 2018
@blink1073 blink1073 added this to the 0.34.x milestone Sep 28, 2018
@blink1073 blink1073 removed this from the 0.34.x milestone Sep 28, 2018
@blink1073 blink1073 added this to the 0.35 milestone Sep 28, 2018
@blink1073 blink1073 removed this from the 0.35 milestone Sep 28, 2018
@blink1073 blink1073 added this to the 1.0 milestone Sep 28, 2018
@blink1073
Copy link
Member

@blink1073 blink1073 commented Oct 17, 2018

ping @tgeorgeux

Copy link
Contributor

@tgeorgeux tgeorgeux left a comment

Looks great to me.

@blink1073 blink1073 merged commit bf9d302 into jupyterlab:master Oct 18, 2018
1 of 2 checks passed
@mgeier mgeier deleted the remove-ansi_up branch Oct 18, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants