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

Cleanup notebook.tex during PDF generation #768

Merged
merged 4 commits into from Mar 1, 2018

Conversation

Projects
None yet
4 participants
@saulshanabrook
Copy link
Contributor

saulshanabrook commented Feb 26, 2018

Currently, when I generate a PDF from a notebook it leaves behind the notebook.tex file in the directory that the notebook is in. This fixes that by generating the tex file in the same temporary directory as the rest of the latex files. It also removes some related dead code. Closes #767

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Feb 26, 2018

It looks like the only change is in a test?

@saulshanabrook saulshanabrook changed the title WIP: Cleanup notebook.tex during PDF generation Cleanup notebook.tex during PDF generation Feb 26, 2018

@saulshanabrook

This comment has been minimized.

Copy link
Contributor Author

saulshanabrook commented Feb 26, 2018

@takluyver It now includes the fix. I added that as a separate commit so that we could verify it did, in fact, fail and that the next commit fixed it.

@saulshanabrook saulshanabrook force-pushed the saulshanabrook:pdf-cleanup-tex branch from 3e187a4 to 6bdba4f Feb 26, 2018

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Feb 26, 2018

Gotcha, thanks.

@Naereen
Copy link

Naereen left a comment

LGTM

@@ -37,4 +37,6 @@ def test_export(self):
(output, resources) = self.exporter_class(latex_count=1).from_filename(newpath)
self.assertIsInstance(output, bytes)
assert len(output) > 0
# tex file should be cleaned up
assert 'notebook.tex' not in os.listdir(td)

This comment has been minimized.

@Naereen

Naereen Feb 26, 2018

That's a great test!

@@ -37,4 +37,6 @@ def test_export(self):
(output, resources) = self.exporter_class(latex_count=1).from_filename(newpath)
self.assertIsInstance(output, bytes)
assert len(output) > 0
# tex file should be cleaned up
assert 'notebook.tex' not in os.listdir(td)

This comment has been minimized.

@mpacer

mpacer Feb 26, 2018

Member

Given that this is also removing the clean_temp_files command, I'd appreciate a check that ensured any of the companion files would also be removed.

This comment has been minimized.

@saulshanabrook

saulshanabrook Feb 27, 2018

Author Contributor

Sure. I updated it to make sure only the initial notebook exists in the directory.

@@ -37,4 +37,6 @@ def test_export(self):
(output, resources) = self.exporter_class(latex_count=1).from_filename(newpath)

This comment has been minimized.

@mpacer

mpacer Feb 26, 2018

Member

I'd feel better if this test would run LaTeX with a bibfile since that can also introduce artifacts and should be tested as part of the smoke test. If this turns out to be too big of a change, then we can move this into a new PR & make a new issue. But, could you give that a shot?

This comment has been minimized.

@saulshanabrook

saulshanabrook Feb 27, 2018

Author Contributor

I can try to do that. Do you think it makes sense to copy these examples files into the repo for the test? https://github.com/jupyter/nbconvert-examples/tree/master/citations I can't seem to find an existing example notebook in this repo.

This comment has been minimized.

@mpacer

mpacer Feb 27, 2018

Member

I think that those might be out of date — given that they were last updated over 3 years ago.

I'm now thinking that there may be no built-in way to do this… which is… odd.

We may need to look into the LaTeX templates to see if we need to include a new bibliography area and traitlet to get that to work. That's starting to sound like a separate PR though.

Could you try to figure out if any of the other code related to this is dead? If you just include assert False in places it should trigger failures in the tests if those parts of the code are actually being tested.

This comment has been minimized.

@saulshanabrook

saulshanabrook Feb 27, 2018

Author Contributor

Looking at the code coverage, it seems like the only lines not executed in the PDF exporting are the win32 related, error related, or for generating the bib.

I am not sure exactly when the code became dead, if there are other files that are affected.

This comment has been minimized.

@mpacer

mpacer Feb 27, 2018

Member

Code coverage actually doesn't get exactly what I was looking for. My ultimate aim was to know which tests were using the bibliography functionality to see how it was being used. I can just do this myself though…

This comment has been minimized.

@mpacer

mpacer Feb 28, 2018

Member

Alright… so I'm fairly sure that right now that this code is only being run in the tests for the nbconvertapp, which are a gigantic pain to debug and investigate because they're hidden behind a subprocess.Popen call to nbconvert as part of the test… which means that pdb just causes everything to fail.

This comment has been minimized.

@mpacer

mpacer Feb 28, 2018

Member

Regardless my investigations left me believing that currently there is nothing that's properly testing the bibliography support… so we should look into that, but that's a separate PR.

This comment has been minimized.

@saulshanabrook

saulshanabrook Feb 28, 2018

Author Contributor

Sounds good! I don't know much about bib support and haven't used it.

@@ -32,9 +32,11 @@ def test_constructor(self):
def test_export(self):
"""Smoke test PDFExporter"""
with tempdir.TemporaryDirectory() as td:
newpath = os.path.join(td, os.path.basename(self._get_notebook()))
file_name = os.path.basename(self._get_notebook())

This comment has been minimized.

@mpacer

mpacer Feb 27, 2018

Member

Nice — that is cleaner.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Mar 1, 2018

OK, it sounds like this is good to go. Thanks @saulshanabrook !

@takluyver takluyver merged commit e59f4ce into jupyter:master Mar 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@takluyver takluyver added this to the 5.4 milestone Mar 8, 2018

@pxhanus pxhanus referenced this pull request Mar 15, 2018

Open

To-Do List #2

0 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment