-
Notifications
You must be signed in to change notification settings - Fork 101
Add —drop-output-type arg #204
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
Conversation
kynan
left a comment
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.
Thanks, this looks sensible on the whole! A couple of suggestions, some of which are nits :)
rgeorgi
left a comment
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.
Thanks for the feedback! Adding changes.
| cell['outputs'] = [output for output in cell['outputs'] if get_size(output) <= max_size] | ||
| if not keep_output_this_cell or keep_output_types: | ||
| cell['outputs'] = [output for output in cell['outputs'] | ||
| if get_size(output) <= max_size |
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.
Yeah, not sure; my instinct is the main use case for keeping an output type would be to preserve graphs/images/whatever in all cases, but that's just a guess based on a teammate's use case. Think I added a line to the Readme, but will confirm.
1. Reflowing Readme to 80 cols, and adding notes on usage 2. Reworking output type filter as method 3. Cleaning up some leftover debug
kynan
left a comment
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.
Thanks, I think this is looking good, only a few cosmetic changes remaining :)
Co-authored-by: Florian Rathgeber <[email protected]>
Co-authored-by: Florian Rathgeber <[email protected]>
Co-authored-by: Florian Rathgeber <[email protected]>
Co-authored-by: Florian Rathgeber <[email protected]>
Co-authored-by: Florian Rathgeber <[email protected]>
|
Believe this addresses the remaining changes |
kynan
left a comment
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.
Thanks! I think this is good to merge now. Apologies for the delay!
I also wanted the feature referenced in #175 and added:
--drop-output-type- when used with--keep-output, drops outputs of the specified type--keep-output-type- overrides--keep-outputand only keeps outputs of the specified typeAlso supports a colon to specify the
namefield, so--drop-output-type stream:stderrwill strip all outputs of typestreamwith namestderr.Also added a test notebook, unit tests, and updated the readme.
Common output types are
stream(for stderr/stdin, e.g. print statements)error(for exceptions in running the cell), andexecute_outputfor evaluated expressions. I'm sure widgets and things like matplotlib have many other types, so this list is not constrained.