refactor: Rename Op to Notification #7590
Conversation
mscolnick
requested review from
Light2Dark,
dmadisetti and
manzt
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Breaking changes detected in the Notifications Schema! |
|
Breaking changes detected in the OpenAPI specification! |
mscolnick
changed the title
|
Breaking changes detected in the Notifications Schema! |
dmadisetti
left a comment
dmadisetti
left a comment
There was a problem hiding this comment.
Nice constant changes
|
Breaking changes detected in the Notifications Schema! |
There was a problem hiding this comment.
Pull request overview
This PR refactors the codebase to rename "Op" (Operation) to "Notification" for outgoing kernel messages, making the terminology clearer and more semantically accurate. The changes are comprehensive, touching messaging infrastructure, runtime, server layers, tests, and API schemas.
Key Changes:
- Renamed
ops.pytonotification.pyand all related classes (e.g.,CellOp→CellNotification) - Updated function names from
broadcast_op→broadcast_notificationandadd_operation→add_notification - Updated property names from
cell_operations→cell_notificationsthroughout the codebase - Updated OpenAPI schemas and generated YAML files to reflect the new naming
Reviewed changes
Copilot reviewed 96 out of 97 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| marimo/_messaging/notification.py | Core rename: Op → Notification base class, all operation classes renamed to end with Notification |
| marimo/_messaging/notification_utils.py | Function rename: broadcast_op → broadcast_notification |
| marimo/_messaging/serde.py | Updated serialization functions to reference NotificationMessage instead of MessageOperation |
| marimo/_server/session/session_view.py | Renamed properties and methods: cell_operations → cell_notifications, add_operation → add_notification |
| tests/* | Updated all test files to use new notification naming |
| marimo/_runtime/runtime.py | Updated runtime to use new notification classes and broadcast function |
| packages/openapi/* | Updated API schemas to reflect new notification naming |
| marimo/_server/api/endpoints/editing.py | Added new UpdateCellIdsRequest class to separate request from notification |
Comments suppressed due to low confidence (1)
tests/_runtime/test_runtime.py:3851
- This assignment to 'creation_request' is unnecessary as it is redefined before this value is used.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Should have at least one output operation and one stale operation | ||
| assert len(md_cell_ops) >= 2 | ||
| assert len(md_cell_notifications) >= 2 |
There was a problem hiding this comment.
Variable stream is not used.
| import marimo._data.models as data | ||
| import marimo._messaging.errors as errors | ||
| import marimo._messaging.ops as ops | ||
| import marimo._messaging.notification as notification |
There was a problem hiding this comment.
Module 'marimo._messaging.notification' is imported with both 'import' and 'import from'.
|
Breaking changes detected in the Notifications Schema! |
|
Breaking changes detected in the OpenAPI specification! |
|
Breaking changes detected in the Notifications Schema! |
|
Breaking changes detected in the OpenAPI specification! |
|
Breaking changes detected in the Notifications Schema! |
This makes outgoing kernel messages (Notifications) much clearer.