You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hi :) Not necessarily a "bug", but more of a suggestion for the API - currently, nbformat.v4.upgrade outputs None if the notebook is already in v4, which might be a reasonable thing to do (to indicate that no change is necessary), but note that papermill does the following
# Upgrade the Notebook to the latest v4 before writing into it
nb = nbformat.v4.upgrade(nb)
without a check for None. This means if the notebook was already in v4 papermill breaks (which I discovered due to, well, papermill breaking!
So the solutions are either to modify papermill, or (and this is where I'd ask for your advice), maybe the upgrade function should just return nb? For what it's worth, this is the current code (nbformat/v4/convert.py, line 73)
elif from_version == 4:
if from_minor == nbformat_minor:
return
but when I delete the return line, Copilot prefers to fill in
elif from_version == 4:
if from_minor == nbformat_minor:
return nb
which might be a somewhat objective way to show that this is the more intuitive API?
The text was updated successfully, but these errors were encountered:
Hi :) Not necessarily a "bug", but more of a suggestion for the API - currently,
nbformat.v4.upgrade
outputs None if the notebook is already in v4, which might be a reasonable thing to do (to indicate that no change is necessary), but note thatpapermill
does the followingwithout a check for None. This means if the notebook was already in v4 papermill breaks (which I discovered due to, well, papermill breaking!
So the solutions are either to modify papermill, or (and this is where I'd ask for your advice), maybe the upgrade function should just return nb? For what it's worth, this is the current code (
nbformat/v4/convert.py
, line 73)but when I delete the return line, Copilot prefers to fill in
which might be a somewhat objective way to show that this is the more intuitive API?
The text was updated successfully, but these errors were encountered: