Support optional hash output in save method

[itsmevichu]

Fix for #1453

[Zsailer]

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 a backwards 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 ContentsManager.save method before it is instantiated in the ServerApp, wrap their method and overload it with the new argument, and raise a FutureWarning telling folks with custom ContentsManagers they will need to update their class soon (before 3.x release someday). This will prevent us from breaking custom contents managers today, release this change in 2.x, and later remove this warning once we release a 3.x.

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!

[krassowski]

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.

Suggested change

	model = await ensure_async(
	self.contents_manager.save(model, path, require_hash=require_hash)
	)
	import inspect
	save = self.contents_manager.save
	signature = inspect.signature(save)
	parameters = signature.parameters
	has_kwargs = any(p.kind == p.VAR_KEYWORD for p in parameters)
	if "require_hash" in parameters or has_kwargs:
	save_coroutine = save(model, path, require_hash=require_hash)
	else:
	save_coroutine = save(model, path)
	model = await ensure_async(save_coroutine)

[krassowski]

I just re-read Zach's comment and it is proposing a more radical approach:

inspecting the method signature of a ContentsManager.save method before it is instantiated in the ServerApp, wrap their method and overload it with the new argument, and raise a FutureWarning telling folks with custom ContentsManagers they 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.

[minrk]

I think we've used backcall 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).

[krassowski]

(backcall was removed from IPython in ipython/ipython#14216)