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

Re-factor ANSI color handling #1230

Merged
merged 5 commits into from
Mar 24, 2016
Merged

Re-factor ANSI color handling #1230

merged 5 commits into from
Mar 24, 2016

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Mar 18, 2016

This is based on a PR I made for nbconvert: jupyter/nbconvert#225.

It also contains the "upgrade" to 16 ANSI colors from a future PR, see jupyter/nbconvert#245 and jupyter/nbconvert#259.

I've not yet updated the tests because I'd like to get some general feedback before.

WARNINGS:

  • I'm new to JavaScript, this is my first non-trivial use of it. Please tell me anything that can be improved or made more idiomatic.
  • I don't know if this has any performance implications
  • The implementation uses some ES6 features, I hope there is some kind of transpiler involved to make this work on older browsers

@mgeier
Copy link
Contributor Author

mgeier commented Mar 19, 2016

When I'm running the tests, I'm getting this (the same happens at Travis CI):

Page Error
  Message:   SyntaxError: Parse error
  Call stack:
Page Error
  Message:   SyntaxError: Parse error
  Call stack:

Does that mean that ES6 is not supported?
Or is it some other problem?

@mgeier mgeier mentioned this pull request Mar 19, 2016
@minrk
Copy link
Member

minrk commented Mar 21, 2016

@mgeier ES6 isn't used in the notebook js yet, so you have to use ES5.

@mgeier
Copy link
Contributor Author

mgeier commented Mar 21, 2016

@minrk OK, will do. What are the plans for ES6 support?

The implementation is based on Python code from
nbconvert.filters.ansi2html().

Among other things, this fixes jupyter#988.
@minrk
Copy link
Member

minrk commented Mar 21, 2016

We are adding it in new projects in various places (JupyterLab, ipywidgets, etc.). I'm not sure if we will add ES6 support to the old notebook js, as it is planned to be replaced by the totally rewritten frontend code in JupyterLab, which is mostly using TypeScript and ES6.

@mgeier
Copy link
Contributor Author

mgeier commented Mar 21, 2016

OK, thanks, that sounds reasonable.

I've re-based my initial commit (7bee6c5).

Then I've removed the ES6 stuff (245287a).
During that, I've also found out that conversion from a string to an integer number is insanely complicated in JavaScript and extended my code a bit. I hope now it's correct, but I've used another regex, so it might have become a bit slower.

Finally, I've updated the tests and added a new test case for overlapping formatting, which wasn't supported before (9d05258).
During that I found a little bug, so adding this test case already payed off.

Now Travis CI doesn't complain anymore, hooray!

I would love to hear some feedback about the new code!

What about the colors? Does everybody like them? Any suggestions for improvements?

if (numbers.length < 2) {
console.log("Not enough fields for VT100 color", numbers);
// Ignored: Invalid color specification
numbers.length = 0;
Copy link
Member

Choose a reason for hiding this comment

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

No need to set length = 0 if we are returning here, plus length should be treated as read-only in general.

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 actually have to clear numbers here, because there might already be some entries from previous loop iterations.

Would you prefer if I used

numbers = [];

Or something entirely different?

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 just saw that there is an error because I'm not returning an Array in this case!

Would it be adequate to do this:

return [];

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... or should I rather use break;?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a different option, I tried to inline this function: db651f7 2bad374

Now the numbers Array is created only once, and never re-assigned.

What do you think about this?
If you don't like it, I can remove this commit again.

@mgeier
Copy link
Contributor Author

mgeier commented Mar 22, 2016

@minrk Thanks for your comments, I've added a commit: 52fae53

@minrk minrk added this to the 5.0 milestone Mar 24, 2016
@minrk minrk merged commit 8d5bac4 into jupyter:master Mar 24, 2016
@minrk
Copy link
Member

minrk commented Mar 24, 2016

Thanks!

@mgeier mgeier deleted the ansicolors branch March 24, 2016 12:27
@mgeier
Copy link
Contributor Author

mgeier commented Mar 24, 2016

Thanks for merging!

@willingc
Copy link
Member

Thanks @mgeier!

@KristofferC
Copy link

Thank you for this, looking forward to the new release.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants