-
Notifications
You must be signed in to change notification settings - Fork 6
Add API to create new FileIDs from the client #36
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
Conversation
| @authorized | ||
| def post(self): | ||
| try: | ||
| path = self.get_argument("path") |
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.
my bad, I did not follow full conversation. why is this a post request? is it because if file Id did not exist, it will be created?
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.
yeah that's right.
| path = self.get_argument("path") | ||
| id = self.file_id_manager.index(path) | ||
| self.write(json_encode({"id": id, "path": path})) | ||
| except web.MissingArgumentError: |
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.
Are there other exceptions that file_id_manager.index can raise that we need to catch?
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.
There are no other specific errors that the file_id_manager raises.
sqlite3 could, in principle, raise an exception, but I don't think we should do anything special to handle that case—just let it raise a 500 and display the error to the user?
|
Also, can't we delete the session_manager that is in main with this PR, or do you want to wait until the frontend is mitragted to use the new endpoint in this PR? |
Do you mean the document session API (not the Jupyter Server session manager)? The Jupyter Server session manager is still needed to mapping the ydoc to kernel client. I still believe that is the correct API for bridging the two. |
| # handler that just adds compatibility with Jupyter Collaboration's frontend | ||
| (r"api/collaboration/session/(.*)", YRoomSessionHandler) | ||
| (r"api/collaboration/session/(.*)", YRoomSessionHandler), | ||
| (r"api/fileid/index", FileIDIndexHandler) |
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.
Will this API be used by UI code?
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.
yep!
No description provided.