- Notifications
You must be signed in to change notification settings - Fork383
Support optional hash output in save method#1454
Support optional hash output in save method#1454itsmevichu wants to merge 3 commits intojupyter-server:mainfrom
Conversation
Zsailer commentedAug 22, 2024
Thank you for the contribution,@itsmevichu! This is great! We discussed this PR a bit in the public meeting today. A couple of things we should consider here... As it's currently written, this introduces abackwards incompatible change to the ContentsManager interface and thus, would require a major version release (granted, we haven't always followed this policy strictly). That said, I would be a bummer to block this PR just because we need to wait for a major release. A 3.x release might not happen for awhile. In the meeting, we discussed ways we could get the goal of this change into the 2.x release cycle. We came up with the idea of inspecting the method signature of a Would you like to explore making these suggested changes? If you don't have time/bandwidth, that's okay too. I (or someone else around the project) can build on this PR and carry it across the finish line. I think@krassowski mentioned having interest in getting this merged before JupyterLab 4.3.x comes out of beta in the next couple of weeks. Thanks! |
| model = await ensure_async( | ||
| self.contents_manager.save(model, path, require_hash=require_hash) | ||
| ) |
krassowskiFeb 14, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
I believe this should fix the backward compatibility@Zsailer had in mind. Import needs to be moved, to top but GitHub UI does not allow to add a suggestion on non-changed part of the file.
| model=awaitensure_async( | |
| self.contents_manager.save(model,path,require_hash=require_hash) | |
| ) | |
| importinspect | |
| save=self.contents_manager.save | |
| signature=inspect.signature(save) | |
| parameters=signature.parameters | |
| has_kwargs=any(p.kind==p.VAR_KEYWORDforpinparameters) | |
| if"require_hash"inparametersorhas_kwargs: | |
| save_coroutine=save(model,path,require_hash=require_hash) | |
| else: | |
| save_coroutine=save(model,path) | |
| model=awaitensure_async(save_coroutine) |
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.
I just re-read Zach's comment and it is proposing a more radical approach:
inspecting the method signature of a
ContentsManager.savemethod before it is instantiated in theServerApp, wrap their method and overload it with the new argument, and raise aFutureWarningtelling folks with customContentsManagersthey will need to update their class soon (before 3.x release someday)
I don't know what is better, will yield to maintainers here. In any case I hope the suggestion is helpful.
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.
I think we've usedbackcall for this in the past for IPython hooks to safely add arguments without breaking anything. I don't have a strong argument for the implementation details, but the pattern is well established and friendly to both maintainers and extension developers. The FutureWarning is good if we want to force this on all ContentsManager implementations, or we can skip it if not supporting the hash feature is a reasonable approach to take (I expect it is).
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.
(backcall was removed from IPython inipython/ipython#14216)
Fix for#1453