-
Notifications
You must be signed in to change notification settings - Fork 567
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
Conversation
It looks like the only change is in a test? |
@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. |
3e187a4
to
6bdba4f
Compare
Gotcha, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice — that is cleaner.
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.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