Bump pyscript#15
Conversation
There was a problem hiding this comment.
Pull Request Overview
Bumps pyscript assets to version 2025.3.1, introduces a custom Element class for DOM manipulation and refactors event handling, and applies updates across Kubernetes name encoding, CLI argument handling, and Mermaid tree rendering.
- Updated pyscript CSS/JS references in
docs/mermaid.html - Added
Elementclass indocs/app.pyand refactoredevent_listener - URL-encoded Kubernetes resource names, enforced required CLI args, and refined namespace ordering in Mermaid output
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/mermaid.html | Updated pyscript links; imported Element for use in snippets |
| docs/app.py | Defined Element class; refactored event_listener to use js |
| d3fendtools/kuberdf/base.py | Added URL-encoding of name via urllib.parse.quote |
| d3fendtools/as_mermaid/main.py | Made input files argument required in CLI |
| d3fendtools/as_mermaid/init.py | Added K8S.Service; enhanced create_tree to defer namespaces |
Comments suppressed due to low confidence (1)
docs/app.py:242
- [nitpick] The event_listener decorator bypasses the new Element class and directly uses js.document; consider using the Element API or removing the unused Element import for consistency.
Element(element_id).element.addEventListener(event_type, create_proxy(callback))
| from pyodide.ffi import create_proxy | ||
| import js | ||
|
|
||
| from app import Element |
There was a problem hiding this comment.
[nitpick] Consider consolidating the import of Element and the import of the app module: you could reference app.Element instead of importing Element separately to reduce redundancy.
| from app import Element |
| self.kind = manifest["kind"] | ||
| self.metadata = manifest["metadata"] | ||
| self.name = self.metadata["name"] | ||
| from urllib.parse import quote |
There was a problem hiding this comment.
Consider moving the urllib.parse.quote import to the top of the module to avoid repeated imports inside init and improve readability.
| from urllib.parse import quote |
|
|
||
| @click.command() | ||
| @click.argument("files", nargs=-1, type=click.Path(exists=True)) | ||
| @click.argument("files", nargs=-1, type=click.Path(exists=True), required=True) |
There was a problem hiding this comment.
click.argument does not support a required=True parameter, which will cause a runtime error. To enforce at least one file, validate the 'files' list in the command body or use nargs=1+.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| if child == parent: | ||
| continue | ||
| if parent_type == K8S.Namespace: | ||
| if (is_namespace, parent_type) == (False, K8S.Namespace): |
There was a problem hiding this comment.
[nitpick] The tuple comparison for namespace handling is non-intuitive; consider rewriting as if not is_namespace and parent_type == K8S.Namespace to express intent more clearly.
| if (is_namespace, parent_type) == (False, K8S.Namespace): | |
| if not is_namespace and parent_type == K8S.Namespace: |
|
@ioggstream I've opened a new pull request, #17, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Initial plan * Fix click.argument usage by removing required=True and adding validation Co-authored-by: ioggstream <1140844+ioggstream@users.noreply.github.com> * Add required=True to --output option to prevent None value Co-authored-by: ioggstream <1140844+ioggstream@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ioggstream <1140844+ioggstream@users.noreply.github.com>
No description provided.