Cleanup notebook.tex during PDF generation #768
Conversation
|
It looks like the only change is in a test? |
saulshanabrook
changed the title
|
@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
force-pushed
the
pdf-cleanup-tex
branch
from
3e187a4
to
6bdba4f
Compare
|
Gotcha, thanks. |
| @@ -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) | |||
| @@ -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) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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…
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()) | |||
|
OK, it sounds like this is good to go. Thanks @saulshanabrook ! |
Currently, when I generate a PDF from a notebook it leaves behind the
notebook.texfile 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