Skip to content
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

Added option --html to convert failed notebooks to html. #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bilke
Copy link

@bilke bilke commented Dec 21, 2021

To be used by CI as build artifacts.

Initial implementation by @chleh.

Copy link
Member

@amit1rrr amit1rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution. --html flag looks like a good addition to treon. I've left some comments.

P.S. - I'll not be able to review again for next 8-10 days as I'm out of office. But will look out for this in early January. Happy Holidays.

try:
processor.preprocess(notebook, metadata(path))
except CellExecutionError:
if html:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like html will only be generated if there's execution error. If there's an HTML flag then I'm guessing user is expecting HTML output even if the notebook executed successfully.

@@ -46,8 +47,9 @@ def main():
setup_logging(arguments)
LOG.info('Executing treon version %s', __version__)
thread_count = arguments['--threads'] or DEFAULT_THREAD_COUNT
html_output = arguments['--html'] or False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe convert to bool i.e. bool(arguments['--html']) so that we're sure it's a boolean.

Let's use is_html as the variable name to indicate it's a boolean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants