Feat/session#1
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the display function to support complex session configurations across local, Google Colab, and JupyterHub environments using POST-into-iframe and URL hash mechanisms. It also adds a build-time hook for environment file packaging. Review feedback suggests improving unique ID generation to avoid collisions, standardizing URL path handling, removing redundant imports, and fixing PEP 8 formatting issues.
| elif is_jupyterhub(): | ||
| display_jupyterhub_post(server.xopat_url, slide, width, height) | ||
| else: | ||
| uid = hashlib.md5(f"{time.time()}".encode()).hexdigest()[:8] |
There was a problem hiding this comment.
Using time.time() as the sole seed for a unique ID can lead to collisions if display() is called multiple times in rapid succession (e.g., within a loop or multiple cells executed together). Consider using uuid.uuid4().hex[:8] for a more robust unique identifier, or at least include os.getpid() in the hash seed.
| raise TypeError( | ||
| "display(): second argument must be a slide id (str) or a session config (dict)" | ||
| ) No newline at end of file |
There was a problem hiding this comment.
The file is missing a newline at the end, which violates PEP 8 style guidelines.
| raise TypeError( | |
| "display(): second argument must be a slide id (str) or a session config (dict)" | |
| ) | |
| raise TypeError( | |
| "display(): second argument must be a slide id (str) or a session config (dict)" | |
| ) | |
| import json as _json | ||
| payload_js = _json.dumps(_json.dumps(session)) # → JS string literal |
There was a problem hiding this comment.
The local import of json as _json is redundant because json is already imported at the module level on line 8. You can use the global json directly.
| import json as _json | |
| payload_js = _json.dumps(_json.dumps(session)) # → JS string literal | |
| payload_js = json.dumps(json.dumps(session)) # → JS string literal |
| import hashlib | ||
| import time | ||
| from IPython.display import HTML, display as _ipy_display |
There was a problem hiding this comment.
These imports should be moved to the top of the file to follow standard Python practices and maintain consistency with other modules in the package (like colab.py).
Deplyoments: first implementation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
I would keep this open and maby polish the code a bit. The idea is to be able to provide the WHOLE session config.