-
Notifications
You must be signed in to change notification settings - Fork 301
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
proposal: merge LargeFileManager into FileContentsManager #899
Comments
Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! 🤗 |
I might be out of scope, but at the last Jupyter Server meeting @afshin suggested using something like WebDAV instead of having our own custom implementation of a contents API. |
Thanks for opening @dlqqq!
Sorry to hear that!
Sure. This seems like a reasonable idea to me. Would you like to make a PR? We should put a deprecation cycle on the class name in case anyone is inheriting from the FileContentsManager and not the LargeFileContentsManager.
I don't think the number lines was ever the reason these classes weren't combined; rather, I think it came from a place of caution when this feature was added (5 years ago). We wanted to allow people to opt-out of the LargeFileContentsManager if they needed to (for whatever reason).
"Separating application logic" wasn't the reason these two classes were separated in the first place—i.e. it wasn't just to make the logic live in separate files so that Jupyter Server developers didn't have to look at many lines of code—rather, it was developed this way to provide two different layers/hooks on which developers could build custom contents managers to extend and configure its functionality. This inherit, extend, and override/configure model is what allows developers to customize Jupyter Server. We provide some default implementations of each manager (at various layers) for developers to build on top of those layers. That said, I agree that this particular example of layering on logic to the ContentsManager seems unnecessary. I don't see much of a use-case for the FileContentsManager. |
#929 reminds me that we should also consider switching the default to the async form of whatever |
Problem
I was tracing the
ContentsManager
implementation to find out how I can attach some logic to its copy/rename/move methods. According the dev docs, the default implementation of theContentsManager
abstract class isFileContentsManager
.After wasting a lot of time looking through
serverapp.py
to find whereFileContentsManager
was being initialized, I found thatFileContentsManager
is not the default implementation, butLargeFileManager
is, initialized here as a default value for thecontents_manager_class
traitlet:Digging through the source, it looks like
LargeFileManager
just extends thesave
method onFileContentsManager
. That got me thinking: is there any reason to keep bothLargeFileManager
andFileContentsManager
in our source tree? I simply don't see a use case where a developer would want to exclusively useFileContentsManager
and ignore handling of large files. The extra level of inheritance makes thesave()
logic harder to trace, and just seems unnecessary. The 2.0 release is a good opportunity to clean up the codebase.Proposed Solution
Merge the two classes together, and drop
LargeFileManager
. Same goes forAsyncLargeFileManager
. More precisely, we can add a private methodFileContentsManager#_save_large_file()
and modify the existingFileContentsManager
to have another block:If we're against adding more logic to the 900-line
FileContentsManager
class, we can migrate file save logic to a helper or util function somewhere else. I'm just against inheritance being the favored approach for separating application logic here.P.S.
One final note: can we drop
"notebook.services.contents.manager.ContentsManager"
fromklasses
in the traitlet definition?The text was updated successfully, but these errors were encountered: