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

Correctly display prompt numbers that are 'None' #6706

Merged
merged 12 commits into from Oct 15, 2014

Conversation

jhamrick
Copy link
Contributor

Fixes #6702

@minrk
Copy link
Member

minrk commented Oct 15, 2014

Would it be possible to add tests to catch regressions?

@jhamrick
Copy link
Contributor Author

I can definitely add tests for the live notebook. I'm not sure what the best way to test for it in the HTML/LaTeX output would be though -- maybe a regex match for the In: and Out: prompts?

@jhamrick
Copy link
Contributor Author

Ok, tests added. The one thing I didn't do though was add tests for the style_bw_ipython.tplx template, because it doesn't seem like it's being used anywhere so I'm not sure how to get nbconvert to use it instead of style_ipython.tplx. I could create a custom template in the tests folder (instead of article.tplx or report.tplx) but that feels hacky somehow?

@takluyver
Copy link
Member

I think it's OK to leave that case untested.

out_regex = r"Out\[(.*)\]:"

ins = ["1", "2", "6", "7", "8", "10", "14", " ", " ", "*", "0"]
outs = ["7", "10", "14"]
Copy link
Member

Choose a reason for hiding this comment

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

I think checking that the prompts exactly match this is a bit brittle, because if you e.g. add one cell for another test, this will break. I'm not sure what's a good way to write the test, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, true. I could create a new notebook in files/ that's specifically for testing prompt numbers, and is therefore less likely to be changed unless the prompt numbering tests are also changed.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@takluyver
Copy link
Member

Can we cut the prompt numbers test notebook down to just enough cells to test the different possibilities for prompt numbers? That just makes it clear what it's there for.

@jhamrick
Copy link
Contributor Author

Yup. Now it just includes cells/tests for: single-digit prompt number, double-digit prompt number, output prompt number, no prompt number, null prompt number, asterisk, and zero prompt number.

@takluyver
Copy link
Member

Excellent, thanks. Merging.

takluyver added a commit that referenced this pull request Oct 15, 2014
Correctly display prompt numbers that are 'None'
@takluyver takluyver merged commit ecca4c5 into ipython:master Oct 15, 2014
@jhamrick
Copy link
Contributor Author

Sweet, thanks!

@jhamrick
Copy link
Contributor Author

Hmm, this is still a problem on nbviewer though because it's running 2.3.0. Should this be backported, or just wait for 3.0?

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Correctly display prompt numbers that are 'None'
@minrk minrk modified the milestone: 3.0 Nov 14, 2014
minrk added a commit to minrk/ipython that referenced this pull request Nov 21, 2014
@minrk minrk added this to the 2.4 milestone Nov 21, 2014
@minrk minrk removed this from the 3.0 milestone Nov 21, 2014
@jhamrick jhamrick deleted the display-prompt-numbers branch February 28, 2015 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cell numbering after ClearOutput preprocessor
3 participants