Skip to content
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

Fix abstract base class definition #33

Merged
merged 2 commits into from
Oct 27, 2022
Merged

Conversation

kevin-bates
Copy link
Member

This pull request updates the abstract base class definition such that the detection of non-implemented abstract methods occurs at class instantiation rather than method invocation. It also adds help strings for each of the abstract methods that didn't already have them. These help-strings attempt to be agnostic to implementation and occasionally imply that some implementations may vary.

There were a couple of changes that are worth pointing out and discussing ...

  1. The root_dir configurable validation varies between Filesystem-based managers and "arbitrary" managers and both None and non-absolute paths should be tolerated in the case of the latter.
  2. The save() method should return the corresponding ID and create an ID if one does not already exist. I.e., it should emulate both create and update operations for consistency. In essence, I believe it's a synonym for index() IIUC.

Resolves: #32

@welcome
Copy link

welcome bot commented Oct 27, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@kevin-bates kevin-bates changed the title Fixup abstract base class definition Fix abstract base class definition Oct 27, 2022
@dlqqq
Copy link
Collaborator

dlqqq commented Oct 27, 2022

@kevin-bates Thank you for the PR! Good catch on the ArbitraryFileIdManager needing to allow non-absolute paths like s3://bucket. I had noticed the same thing on my branch.

The save() method should return the corresponding ID and create an ID if one does not already exist. I.e., it should emulate both create and update operations for consistency. In essence, I believe it's a synonym for index() IIUC.

Not quite. save() does nothing if the file has not yet been indexed. It's really an implementation defined method that updates any data associated with a file ID whenever it receives a save event. This is only relevant to LocalFileIdManager, which needs to update the timestamps associated with a file ID, so it doesn't think that the file was deleted and a new one was created at the same path.

@dlqqq
Copy link
Collaborator

dlqqq commented Oct 27, 2022

IK the docs for save() are not correct, but we need to do a more extensive documentation overhaul anyways once things get less busy.

@dlqqq dlqqq merged commit 52d13f0 into jupyter-server:main Oct 27, 2022
@welcome
Copy link

welcome bot commented Oct 27, 2022

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

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.

make BaseFileIdManager a true ABC
2 participants