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.

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

Enable ANSI underline and inverse #696

Merged
merged 5 commits into from
Feb 2, 2018
Merged

Enable ANSI underline and inverse #696

merged 5 commits into from
Feb 2, 2018

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Oct 23, 2017

This is a companion PR to jupyter/notebook#2967.

@mpacer
Copy link
Member

mpacer commented Oct 25, 2017 •
edited
Loading

You're going to have to give a lot more context than just saying it is a companion.

What is the purpose of this particular change? Please explain for those who will need to debug this in the future.

Note: you made an API breaking change, these are private methods but still… we try to avoid breaking APIs. Why does it need to be a positional argument?

You removed some documentation, is that documentation wrong? If it is, is it that the functionality should change or the documentation should change?

I am not a fan of the multiple lines of documentation that describe different pieces of functionality. Please break them up into individual lines for the individual code definitions.

@mgeier
Copy link
Contributor Author

mgeier commented Oct 26, 2017

@mpacer Thanks for the feedback!

You're going to have to give a lot more context than just saying it is a companion.

I wanted to wait for feedback on the notebook issue and if and when that gets accepted, this PR is just about implementing the same thing for nbconvert.

What is the purpose of this particular change? Please explain for those who will need to debug this in the future.

Do you mean here in the Github comments or somewhere in the code docstring/comments?

The purpose is in the title of this PR.

The notebook already supports ANSI "underline", and it somewhat supports "inverse", but instead of inverting foreground and background, a dotted line is drawn around the text. In jupyter/notebook#2967 I'm trying to actually invert the colors instead.

You removed some documentation, is that documentation wrong?

Yes, since #259.

If it is, is it that the functionality should change or the documentation should change?

The functionality did change in #259 and the documentation should follow suit at some point.

I am not a fan of the multiple lines of documentation that describe different pieces of functionality. Please break them up into individual lines for the individual code definitions.

I don't understand what you mean by that.

It would probably be best to get rid of most of the documentation, because it will not be as accurate as the code anyway, and it is very likely to fall out of sync (which it already did).

What about removing the description about the actual codes from the docstring and instead adding a few comments in the code where it is not obvious what certain codes do?

@mgeier
Copy link
Contributor Author

mgeier commented Oct 26, 2017

I forgot to address this:

Note: you made an API breaking change, these are private methods but still… we try to avoid breaking APIs. Why does it need to be a positional argument?

It doesn't need to be a positional argument. I can make it a keyword argument if that's what you want, but it won't really help. The problem is that I'm calling the converter functions with one more argument in _ansi2anything(). If somebody uses _ansi2anything() with their own converter function, the keyword argument won't help.

But as you say, those are private functions. And I made them private intentionally. They are only there as an implementation detail for internal code re-use and they are not meant to be used externally.

In the future, we might even add more ANSI formatting, which will require further "breaking" changes.

@mgeier
Copy link
Contributor Author

mgeier commented Nov 2, 2017

jupyter/notebook#2967 was merged, I've updated this PR accordingly: 006153c

I don't know when/how the new CSS classes from the notebook will be available to nbconvert. If you need me to add temporary CSS definitions, please tell me where I should add them.

mgeier added a commit to spatialaudio/nbsphinx that referenced this pull request Nov 2, 2017
@mgeier
Enable default colors for ANSI "inverse"
3df1324 
This will work once jupyter/nbconvert#696 is
available.
@mgeier mgeier mentioned this pull request Nov 2, 2017
Merged
@mgeier
Copy link
Contributor Author

mgeier commented Nov 30, 2017

@mpacer Ping?

@takluyver
Copy link
Member

I think this looks OK, and I'm not worried about changing the API of these private functions.

Can you add a few tests for the new functionality in nbconvert.filters.tests.test_ansi?

@mgeier
Copy link
Contributor Author

mgeier commented Jan 5, 2018

@takluyver

I've shortened a docstring (a6e4ba2) as mentioned in #696 (comment).

And I've added some test cases in e443e85.

What about the new CSS classes?

@mgeier mgeier force-pushed the ansi-underline-inverse branch from e443e85 to 197dcee Compare January 7, 2018 11:58
@mgeier
Copy link
Contributor Author

mgeier commented Jan 7, 2018

I've changed the last commit to avoid a problem with Python 2.x and r'\u' (197dcee) and now the tests on Travis-CI pass.

@takluyver
Copy link
Member

Grant is getting ready to release notebook 5.3 in the next couple of days, which will include your PR jupyter/notebook#2967 . Once that's done, you can update the version of the notebook it gets the CSS from in setup.py, as part of this PR.

@takluyver
Copy link
Member

Thanks!

@mpacer do you want to have another look at this before we press the green button?

@takluyver takluyver mentioned this pull request Jan 17, 2018
Open
@mgeier
Copy link
Contributor Author

mgeier commented Jan 31, 2018

Ping?

@mgeier mgeier force-pushed the ansi-underline-inverse branch from fe17cf1 to 4bc6c2e Compare February 1, 2018 09:36
@mgeier
Copy link
Contributor Author

mgeier commented Feb 1, 2018

I've rebased this PR onto master to avoid a conflict where I updated the Jupyter CSS to 5.3.0 while in the meantime it was updated to 5.4.0 in #748.

@mgeier mgeier mentioned this pull request Feb 1, 2018
Closed
takluyver
takluyver reviewed Feb 1, 2018
View reviewed changes
nbconvert/filters/tests/test_ansi.py
@@ -61,6 +64,7 @@ def test_ansi2latex(self):
'hello\x1b[01;34mthere': r'hello\textcolor{ansi-blue-intense}{\textbf{there}}',
'hello\x1b[001;34mthere': r'hello\textcolor{ansi-blue-intense}{\textbf{there}}',
'\x1b[1mhello\x1b[33mworld\x1b[0m': r'\textbf{hello}\textcolor{ansi-yellow-intense}{\textbf{world}}',
'he\x1b[4mll\x1b[24mo': 'he\\underline{ll}o',
Copy link
Member

Choose a reason for hiding this comment

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

Is it convenient to test the conversion of inverted mode to latex as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a bit unwieldy, but why not: dab9596

@takluyver takluyver added this to the 5.4 milestone Feb 2, 2018
@takluyver takluyver merged commit aa20c50 into jupyter:master Feb 2, 2018
@takluyver
Copy link
Member

Thanks!

@mgeier mgeier deleted the ansi-underline-inverse branch February 2, 2018 11:05
mgeier added a commit to spatialaudio/nbsphinx that referenced this pull request Feb 3, 2019
@mgeier
Enable default colors for ANSI "inverse"
61ae67e 
This will work once jupyter/nbconvert#696 is
available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@takluyver takluyver takluyver left review comments

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

Successfully merging this pull request may close these issues.
3 participants
@mgeier @mpacer @takluyver