-
Notifications
You must be signed in to change notification settings - Fork 10
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
Upload pdf to s3 #38
Upload pdf to s3 #38
Conversation
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, few minor fixes and good to merge.
if __name__ == "__main__": | ||
os.system("mkdir " + PDF_PATH) | ||
html_to_pdf() | ||
merge_pdf() | ||
compile_information_pages() | ||
merge_pdf_pages(['storage/title.pdf', 'storage/acknowledgement.pdf', 'storage/contents.pdf', 'storage/full_pdf.pdf']) | ||
merge_pdf_pages(['storage/title.pdf', 'storage/acknowledgement.pdf', 'storage/contents.pdf', 'storage/merged.pdf']) | ||
upload_file('storage/full_pdf.pdf', 'deepchemtutorials', 'TutorialsBook.pdf') |
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.
pls, keep an empy line at the end
|
||
|
||
if __name__ == "__main__": | ||
unittest.main() |
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.
same here, pls add an empty last line.
@@ -5,6 +5,7 @@ | |||
- pdfunite | |||
- pdfkit | |||
- mdpdf | |||
- boto3 |
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.
please add boto3 to requirements file
@@ -0,0 +1,42 @@ | |||
name: Build latest version of PDF Book | |||
|
|||
on: |
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.
Looks good, can you please explain when exactly this workflow will get triggered.
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.
Yes, the workflow will be triggered whenever new commits are performed in examples/tutorials directory in deepchem repository.
import json | ||
|
||
|
||
class TestConvertHTMLToPDF(unittest.TestCase): |
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.
Add a docstring to this class (use numpydoc style)
|
||
class TestConvertHTMLToPDF(unittest.TestCase): | ||
|
||
def test_convert_html_to_pdf(self): |
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.
Add docstrings to methods
os.rmdir("mocks/mock_temp_storage") | ||
|
||
|
||
class TestMergePDF(unittest.TestCase): |
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.
Docs for this class
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.
Can you please update the README? Make suer to explain how these new capabilities work. Also please add docstrings in numpydocs style .
Why do we need to add mock-html notebooks? We didn't need these previously
Since the function html_to_pdf converts the html files to pdf, we need some mock html files to unit test this functionality. So for testing that I have added mock-html notebooks folder which contains sample html-files. |
Title: Automated Book build and Upload to S3.
Changes Made: