[client] add priority support #25
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android_sms_gateway/domain.py (1)
22-29: Unwrap Enum values in Message.asdict for consistency with Webhook.asdict.Currently priority (IntEnum) lands in the dict as an IntEnum instance (JSON encodes fine), but unwrapping Enums to their
.valuekeeps asdict behavior consistent across models and avoids surprises for non-JSON consumers.Apply this diff to asdict (and import enum):
+import enum @@ def asdict(self) -> t.Dict[str, t.Any]: - return { - snake_to_camel(field.name): getattr(self, field.name) - for field in dataclasses.fields(self) - if getattr(self, field.name) is not None - } + result: t.Dict[str, t.Any] = {} + for field in dataclasses.fields(self): + value = getattr(self, field.name) + if value is None: + continue + if isinstance(value, enum.Enum): + value = value.value + result[snake_to_camel(field.name)] = value + return result
🧹 Nitpick comments (3)
android_sms_gateway/enums.py (1)
33-39: Add brief member-level docs for semantics (e.g., scheduling/queue behavior).Helps consumers understand MINIMUM/DEFAULT/BYPASS_THRESHOLD/MAXIMUM effects on server-side handling.
android_sms_gateway/domain.py (1)
22-22: Optionally coerce raw ints to MessagePriority in post_init.Guards callers passing plain ints and provides a clear error on invalid ranges while keeping the dataclass frozen.
Add this to Message:
def __post_init__(self): if isinstance(self.priority, int) and not isinstance(self.priority, MessagePriority): try: object.__setattr__(self, "priority", MessagePriority(self.priority)) except ValueError as e: raise ValueError("priority must be a valid MessagePriority") from etests/test_domain.py (1)
140-226: Add edge-case cases for DEFAULT and MINIMUM priorities.Covers 0 and negative bounds to lock in serialization behavior.
Append to the param list:
@@ ), + ( + "Hello, world!", + ["123", "456"], + True, + False, + "msg_123", + None, + None, + MessagePriority.DEFAULT, + { + "message": "Hello, world!", + "phoneNumbers": ["123", "456"], + "withDeliveryReport": True, + "isEncrypted": False, + "id": "msg_123", + "priority": 0, + }, + ), + ( + "Hi", + ["555"], + True, + False, + None, + None, + None, + MessagePriority.MINIMUM, + { + "message": "Hi", + "phoneNumbers": ["555"], + "withDeliveryReport": True, + "isEncrypted": False, + "priority": -128, + }, + ),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
android_sms_gateway/domain.py(2 hunks)android_sms_gateway/enums.py(1 hunks)tests/test_domain.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_domain.py (2)
android_sms_gateway/enums.py (1)
MessagePriority(33-39)android_sms_gateway/domain.py (3)
Message(13-29)asdict(24-29)asdict(96-106)
android_sms_gateway/domain.py (1)
android_sms_gateway/enums.py (1)
MessagePriority(33-39)
🔇 Additional comments (3)
android_sms_gateway/enums.py (1)
33-39: LGTM: MessagePriority is well-scoped and uses IntEnum appropriately.Clear range and sensible defaults. No blocking issues.
android_sms_gateway/domain.py (1)
4-4: LGTM: Import of MessagePriority is correct and localized.tests/test_domain.py (1)
3-4: LGTM: Imports reflect new public API surface.
WalkthroughAdds a new MessagePriority IntEnum and an optional priority field to the Message dataclass; Message.asdict now omits None fields and serializes priority as an integer when present. Tests were added to validate serialization; a VS Code test setting file was also added. Changes
Sequence Diagram(s)N/A Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
capcom6
force-pushed
the
client/priority-support
branch
from
9056d61 to
bb7d18b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android_sms_gateway/domain.py (1)
38-49: Serialize enums to primitives in asdict (priority currently emitted as Enum).asdict returns the Enum object for priority; emit its primitive value for consistency (similar to Webhook.event.value) and to avoid downstream surprises.
Apply this change to asdict:
def asdict(self) -> t.Dict[str, t.Any]: - """ - Returns a dictionary representation of the message. - - Returns: - Dict[str, Any]: A dictionary representation of the message. - """ - return { - snake_to_camel(field.name): getattr(self, field.name) - for field in dataclasses.fields(self) - if getattr(self, field.name) is not None - } + """ + Returns a dictionary representation of the message. + Enum fields are serialized to their underlying values. + + Returns: + Dict[str, Any]: A dictionary representation of the message. + """ + result: t.Dict[str, t.Any] = {} + for field in dataclasses.fields(self): + value = getattr(self, field.name) + if value is None: + continue + # IntEnum (and Enum) -> primitive for JSON friendliness + if hasattr(value, "value"): + try: + value = value.value # Enum/IntEnum + except Exception: + pass + result[snake_to_camel(field.name)] = value + return resultIf you prefer a stricter type check, import enum and use isinstance(value, enum.Enum).
🧹 Nitpick comments (1)
android_sms_gateway/domain.py (1)
14-26: Docstring LGTM; tiny clarity nit.Consider noting the valid numeric range for priority (-128..127) to match MessagePriority.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.vscode/settings.json(1 hunks)android_sms_gateway/domain.py(3 hunks)android_sms_gateway/enums.py(1 hunks)tests/test_domain.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_domain.py
- android_sms_gateway/enums.py
🧰 Additional context used
🧬 Code graph analysis (1)
android_sms_gateway/domain.py (1)
android_sms_gateway/enums.py (1)
MessagePriority(33-46)
🔇 Additional comments (2)
android_sms_gateway/domain.py (2)
4-4: Import looks good.MessagePriority is used by the new field; import is correct.
36-36: Optional priority field is backward‑compatible.Because None fields are omitted in asdict, older servers won’t see the key unless set. Good design.
Please confirm the upstream API expects the camelCase key "priority" with an integer value.
Summary by CodeRabbit
New Features
Tests
Chores