-
Notifications
You must be signed in to change notification settings - Fork 123
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
jupyter V2.1 with backwards compatible changes #917
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
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, but undelete %incr_cell_to_module
Co-authored-by: Stefan Krawczyk <stefan@dagworks.io>
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. I assume you've tested it, e.g. %incr_cell_to_module still works? :)
…c/hamilton into plugins/jupyter-v2.1
Cool - I'd squash merge this PR when it's ready. |
Reimplementation of #846 with backwards compatible changes with V1.
Compared to V2:
module_name
as positional or named arg (-m/--module_name
)-c/--config
-h/--help
-r/--rebuild-drivers
-v/--verbose
-c/--config
%%insert_module
to%%module_to_cell
Currently ignores the
%%incr_cell_to_module
. We have to consider what it means to deduplicate these 100+ lines of logic. If this feature truly adds value, it should be an argument instead of a separate magic. I propose-a/--append INDEX
. This means the magic would now require to be stateful though opening opportunities for more bugs.How I tested this
The notebook
tutorial.ipynb
covers all implemented functionalities, so it's a good test for intended behavior.TODO
%%cell_to_module
%%incr_cell_to_module