Skip to content

Add format parameter when receiving/creating or updating notebook definitions #560

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

Merged
merged 7 commits into from
May 12, 2025

Conversation

lBilali
Copy link
Contributor

@lBilali lBilali commented Mar 12, 2025

adding format parameter for get_notebook_definition, create_notebook and update_notebook_definition

when format parameter is not provided the notebook definition should be GIT friendly
if format = 'ipynb' is provided then the definition should be standard ipynb which is a json file

fixed issues with encoding the content within create_notebook and update_notebook_definition functions

create_notebook now by default has format=None which is a braking change

@lBilali
Copy link
Contributor Author

lBilali commented Mar 21, 2025

Hei,
Any thoughts on this?

@m-kovalsky
Copy link
Collaborator

Hei, Any thoughts on this?

This will need to wait until after fabcon. Only bugfixes and features relevant to fabcon are being taken up now.

@a-holm
Copy link

a-holm commented Mar 30, 2025

Thanks for adding the format parameter and fixing the encoding issues! This looks like a valuable addition for managing different notebook formats.

A few minor points:

  • You fixed a small, unrelated typo fix in _helper_functions.py. While harmless, mentioning minor cleanups like this in the description (or separating them) is good practice. Might make it easier for the reviewers.
  • It looks like you also fixed a potential path issue in create_notebook and update_notebook_definition (changing notebook-content..{type} to notebook-content.{type}). Good catch! Just wanted to mention it, seems tricky to find.

@lBilali
Copy link
Contributor Author

lBilali commented May 9, 2025

hi @m-kovalsky ,
Did you have any time to look at this?

thanks

Copy link
Collaborator

@m-kovalsky m-kovalsky left a comment

Choose a reason for hiding this comment

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

just a simple code-simplification. then will approve & merge.

Copy link
Collaborator

@m-kovalsky m-kovalsky 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!

@m-kovalsky m-kovalsky merged commit 8061268 into microsoft:main May 12, 2025
1 of 2 checks passed
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.

3 participants