Closed wseaton closed 4 years ago
Hi @wseaton wave ! Thanks for the PR. Left a few inline comments. Do you have the bandwidth to turn your pre_save and post_save examples into tests?
Sure, I'll try to get something out this week :slightly_smiling_face:
@ericdill ping, I added tests for the example hooks in tests/test_s3manager.py
. Let me know if there's a better way to test than just saving a notebook and inspecting the result of the example functions, I think if we wanted to verify the functions were called with the proper signature we'd need to rig up something with unittest.Mock
. Also do you know what's up with the linter error? Thanks!
Linter is complaining about
./s3contents/tests/test_s3manager.py:77:1: E305 expected 2 blank lines after class or function definition, found 1
./s3contents/tests/hooks.py:4:1: E302 expected 2 blank lines, found 1
You should run make fmt
before committing. I think there's also a black formatter check in there that needs to pass. Sorry to let this one die on the vine a bit. Lost track of it last week.
Running this locally works for everything:
make env
conda activate s3contents
make minio
Then in a diff terminal
conda activate s3contents
make test-all
Looks like there's a bunch of warnings, but I'm not going to lay those at your feet 😁 .
Just get that linter appeased and I can merge it (or give me commit to your branch and I can just push the commit)
No worries @ericdill, I'll fix those two issues and add a doc section with the examples from the tests and we should be good.
@ericdill this should be ready to go, just needs your approval on the docs changes!
lgtm. Thanks @wseaton !
This fixes #70 and #65, basically just ported over the
FileSystemContentsManager
implementations from upstream notebook.Tested the following hooks locally with
minio
:Post save hooks are a little tricky to write, but I was able to get one to work that runs
nbconvert
and saves the notebook as HTML by reusingcontents_manager.fs
. I treated theos_path
from the hook function as essentially the S3 API path: