[client] add device management, settings, logs, and health check methods#40
Merged
Conversation
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 58 minutes and 28 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughExpanded client library: moved message endpoints to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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)
364-383:⚠️ Potential issue | 🔴 Critical
refresh_tokentype annotation contradictsfrom_dictand is a breaking change.
refresh_token: stris declared as non-optional (Line 364), butfrom_dictusespayload.get("refreshToken")(Line 382), which returnsNonewhen the server omits the field — producing aTokenResponsewhose annotation is violated. Additionally, making this a required non-default field afterexpires_atbreaks any existing callers that constructTokenResponsepositionally or via kwargs without a refresh token. The README (Lines 443) correctly documents it asOptional[str] = None.🛠️ Suggested fix
- refresh_token: str - """The refresh token.""" + refresh_token: t.Optional[str] = None + """The refresh token."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android_sms_gateway/domain.py` around lines 364 - 383, The TokenResponse class's refresh_token field should be optional with a default to match from_dict and existing callers: change the refresh_token annotation to Optional[str] and give it a default of None (e.g., refresh_token: Optional[str] = None) in the TokenResponse dataclass/definition so payload.get("refreshToken") remains valid and positional/kw callers aren't broken; update any imports (typing.Optional) and ensure TokenResponse.from_dict still uses payload.get("refreshToken") unchanged.
🧹 Nitpick comments (2)
android_sms_gateway/domain.py (1)
262-305: Extract the ISO timestamp parser to reduce repetition.The
fromisoformat(... .replace("Z", "+00:00"))block is repeated 4 times here and again inLogEntry.from_dict. A small helper would improve readability and avoid drift.♻️ Example helper
def _parse_iso(value: t.Optional[str]) -> t.Optional[datetime.datetime]: if value is None: return None return datetime.datetime.fromisoformat(value.replace("Z", "+00:00"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android_sms_gateway/domain.py` around lines 262 - 305, Extract the repeated ISO timestamp parsing into a small helper (e.g., _parse_iso) and replace the four duplicated blocks in Device.from_dict (and the similar blocks in LogEntry.from_dict) with calls to that helper; specifically add a function that accepts Optional[str] and returns Optional[datetime.datetime] by handling None and doing the value.replace("Z", "+00:00") then datetime.fromisoformat, and update Device.from_dict to call _parse_iso(payload.get("createdAt")), _parse_iso(payload.get("updatedAt")), _parse_iso(payload.get("deletedAt")), and _parse_iso(payload.get("lastSeen")) (and do the same refactor in LogEntry.from_dict).android_sms_gateway/client.py (1)
189-189:filtershadows the Python built-in.Ruff A002 at Line 189 and Line 610. Consider renaming (e.g.
queryorfilter_) to avoid confusion; it also affects call-site ergonomics since users must pass it as a keyword anyway.- filter: t.Optional[domain.MessagesQueryFilter] = None, + query: t.Optional[domain.MessagesQueryFilter] = None,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android_sms_gateway/client.py` at line 189, The parameter name filter in the function signature (typed as t.Optional[domain.MessagesQueryFilter]) shadows the built-in and triggers Ruff A002; rename the parameter to a non-shadowing name (e.g., filter_ or query) in the function/method definition in client.py and update all internal references, type hints, and call-sites (including the other occurrence flagged at the second location) to use the new name; ensure exported API/ callers use a keyword-compatible name and run tests/linter to verify no remaining shadowing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android_sms_gateway/client.py`:
- Around line 159-162: The client now calls the renamed endpoints (/messages and
/messages/{id}) in send and get_state (refer to methods send and get_state),
which is a breaking change for servers still using /message; add a prominent
breaking-change entry to your release notes (CHANGELOG.md or
BREAKING_CHANGES.md) that states the endpoint rename, lists affected client
methods (send, get_state), and specifies the minimum compatible SMSGate server
version required (and any migration guidance for operators of older servers).
Ensure the entry is visible in the release headline and include an explicit note
that users on older servers will receive 404s if they upgrade the client without
upgrading the server.
- Around line 145-165: The code is passing params= to our HTTP client which
doesn't accept it; update the HttpClient and AsyncHttpClient protocol method
signatures (post and get) to include params: Optional[Dict[str, str]] = None,
then modify all concrete implementations (RequestsHttpClient, HttpxHttpClient,
AiohttpHttpClient) to accept that params argument and forward it to the
underlying library call (e.g., requests.post/get, httpx.post/get, aiohttp
session methods) using the appropriate query param parameter, and keep send(),
get_messages(), and get_logs() (and their async variants) unchanged so their
params dicts are accepted and sent through.
- Around line 186-213: Update the mismatched docstrings to reflect the actual
function signatures: in get_messages (and the async counterpart around Lines
607-621, e.g., async get_messages_async or similarly named method), replace the
Args section that lists from_, to, state, device_id, limit, offset (and any
nonexistent request param) with descriptions for filter: MessagesQueryFilter and
pagination: QueryPagination, and ensure the Returns section matches
t.List[domain.MessageState] (or an awaitable returning that type for the async
method); remove any references to nonexistent parameters and keep
examples/formatting consistent with other methods in the module.
- Around line 492-497: The refresh_token call currently invokes self.http.post
without the required payload argument and will raise TypeError; update the
synchronous refresh_token (the call wrapped by domain.TokenResponse.from_dict)
to pass an empty dict as the payload (self.http.post(..., {} , headers=...)) so
the Authorization header is sent with an empty body, and make the same change
for the async counterpart (the async refresh_token/refresh_token_async that
calls self.http.post) to pass {} as the payload as well.
In `@android_sms_gateway/domain.py`:
- Around line 400-516: The Settings asdict() methods currently emit snake_case
keys; update each Settings class (SettingsGateway.asdict,
SettingsEncryption.asdict, SettingsMessages.asdict, SettingsLogs.asdict,
SettingsPing.asdict, SettingsWebhooks.asdict) to emit camelCase wire keys (e.g.,
cloudUrl, privateToken, notificationChannel, passphrase, limitPeriod,
limitValue, logLifetimeDays, processingOrder, sendIntervalMax, sendIntervalMin,
simSelectionMode, lifetimeDays -> logLifetimeDays as requested, intervalSeconds,
retryCount, signingKey, internetRequired); keep existing behavior for optional
fields and enum .value usage but change the dictionary keys to the camelCase
names used across the module so from_dict()/API wiring remains consistent.
In `@README.md`:
- Around line 310-313: The README's get_messages signature is outdated and will
cause a TypeError; update the README entry to match the actual implementation by
replacing the old signature with get_messages(*, filter: MessagesQueryFilter =
None, pagination: QueryPagination = None) and add a minimal example showing how
to construct a MessagesQueryFilter and QueryPagination and call
APIClient.get_messages / AsyncAPIClient.get_messages; reference the actual
client method names (APIClient.get_messages, AsyncAPIClient.get_messages) and
the types (MessagesQueryFilter, QueryPagination) so readers know which objects
to import and use.
- Line 310: Update the README entry for the send method to indicate that
skip_phone_validation and device_active_within are keyword-only parameters (must
be passed as kwargs) to match the client.py signature where these appear after a
'*' (function send(message, skip_phone_validation=False,
device_active_within=0)). Edit the table row that documents send (returning
domain.MessageState) to explicitly show the parameters as keyword-only (e.g.,
send(message, *, skip_phone_validation=False, device_active_within=0) or add a
parenthetical note) so users do not attempt to call skip_phone_validation or
device_active_within positionally.
---
Outside diff comments:
In `@android_sms_gateway/domain.py`:
- Around line 364-383: The TokenResponse class's refresh_token field should be
optional with a default to match from_dict and existing callers: change the
refresh_token annotation to Optional[str] and give it a default of None (e.g.,
refresh_token: Optional[str] = None) in the TokenResponse dataclass/definition
so payload.get("refreshToken") remains valid and positional/kw callers aren't
broken; update any imports (typing.Optional) and ensure TokenResponse.from_dict
still uses payload.get("refreshToken") unchanged.
---
Nitpick comments:
In `@android_sms_gateway/client.py`:
- Line 189: The parameter name filter in the function signature (typed as
t.Optional[domain.MessagesQueryFilter]) shadows the built-in and triggers Ruff
A002; rename the parameter to a non-shadowing name (e.g., filter_ or query) in
the function/method definition in client.py and update all internal references,
type hints, and call-sites (including the other occurrence flagged at the second
location) to use the new name; ensure exported API/ callers use a
keyword-compatible name and run tests/linter to verify no remaining shadowing.
In `@android_sms_gateway/domain.py`:
- Around line 262-305: Extract the repeated ISO timestamp parsing into a small
helper (e.g., _parse_iso) and replace the four duplicated blocks in
Device.from_dict (and the similar blocks in LogEntry.from_dict) with calls to
that helper; specifically add a function that accepts Optional[str] and returns
Optional[datetime.datetime] by handling None and doing the value.replace("Z",
"+00:00") then datetime.fromisoformat, and update Device.from_dict to call
_parse_iso(payload.get("createdAt")), _parse_iso(payload.get("updatedAt")),
_parse_iso(payload.get("deletedAt")), and _parse_iso(payload.get("lastSeen"))
(and do the same refactor in LogEntry.from_dict).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 222fe3a9-ca76-40d6-b9d4-b6ecb74c9f4a
📒 Files selected for processing (7)
README.mdandroid_sms_gateway/__init__.pyandroid_sms_gateway/client.pyandroid_sms_gateway/domain.pyandroid_sms_gateway/enums.pydev-docsuser-docs
capcom6
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
android_sms_gateway/client.py (2)
911-916:⚠️ Potential issue | 🔴 CriticalAsync
refresh_tokenstill missingpayloadargument —TypeErrorat runtime.The sync counterpart (Lines 488–494) was fixed to pass
payload={}, but the async version here was not.AsyncHttpClient.postrequirespayload(it's a positional parameter in the protocol and all concrete implementations). Any call toawait client.refresh_token(...)will raiseTypeError: post() missing 1 required positional argument: 'payload'.🛠️ Proposed fix
return domain.TokenResponse.from_dict( await self.http.post( f"{self.base_url}/auth/token/refresh", + payload={}, headers=headers, ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android_sms_gateway/client.py` around lines 911 - 916, The async method refresh_token is calling self.http.post without the required payload argument, which causes a TypeError at runtime; update the call in refresh_token so the await self.http.post(...) includes payload={} (keeping headers and URL unchanged) before passing the response into domain.TokenResponse.from_dict; reference the async refresh_token method, AsyncHttpClient.post requirement, and domain.TokenResponse.from_dict to locate and fix the call.
609-617:⚠️ Potential issue | 🟡 MinorAsync
get_messagesdocstring is out of sync with the signature.Docstring's Args still says
request: The query request containing filters and pagination parameters., but the parameters are actuallyquery: MessagesQueryFilterandpagination: QueryPagination(same wording fix was applied to the sync variant above but not here).📝 Suggested fix
Args: - request: The query request containing filters and pagination parameters. + query: Optional query filter (date range, state, device ID). + pagination: Optional pagination (limit, offset).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android_sms_gateway/client.py` around lines 609 - 617, The async get_messages docstring is outdated: update the Args section to match the function signature by replacing the generic `request` description with entries for `query: MessagesQueryFilter` and `pagination: QueryPagination`, briefly describing each (query filters and pagination params) and keep the Returns description as-is; locate the docstring inside the async def get_messages(...) function and adjust only the parameter names/descriptions to match the signature.
🧹 Nitpick comments (3)
android_sms_gateway/domain.py (1)
668-691:LogEntry.created_attype/parser mismatch.
created_atis annotated as non-optionaldatetime.datetime, but_parse_iso(payload["createdAt"])can returnNonewhen the value isNone. Either make the fieldOptional[datetime.datetime]or inline a non-nullable parse here (e.g.,datetime.datetime.fromisoformat(payload["createdAt"].replace("Z", "+00:00"))).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android_sms_gateway/domain.py` around lines 668 - 691, The LogEntry.from_dict uses _parse_iso(payload["createdAt"]) which can return None but LogEntry.created_at is typed as datetime.datetime; fix by either (A) making the dataclass field LogEntry.created_at Optional[datetime.datetime] and accept None from _parse_iso, or (B) enforce a non-nullable parse inside from_dict (e.g., parse payload["createdAt"] with datetime.fromisoformat after replacing "Z" with "+00:00" and raise/handle if it's missing) so that created_at remains a datetime; update the LogEntry.created_at annotation and any downstream code accordingly and reference the from_dict method and _parse_iso function when making the change.android_sms_gateway/client.py (2)
160-160: Avoid appending a trailing?when there are no query params.When no params are set,
urlencode({})yields""and the request URL becomes e.g./messages?. This is harmless on most servers but ugly in logs and not all proxies/WAFs handle it gracefully. Affectssend,get_messages,get_logsin both sync and async clients. A small helper (or conditional concatenation) would keep URLs clean:qs = urlencode(params) url = f"{self.base_url}/messages" + (f"?{qs}" if qs else "")Also applies to: 214-214, 389-389, 575-575, 630-630, 812-812
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android_sms_gateway/client.py` at line 160, The URL construction currently appends "?{urlencode(params)}" unconditionally which yields a trailing '?' when params is empty; update the callers in client.py (methods send, get_messages, get_logs for both sync and async clients) to build the query string first (qs = urlencode(params)) and then append it only if non-empty (e.g., url = f"{self.base_url}/messages" + (f"?{qs}" if qs else "")), replacing the direct f"{self.base_url}/messages?{urlencode(params)}" usage at the affected spots.
393-405:health_checkdocstring mentions "readiness probe" but that's now a separate method.Since
readiness_check()exists as a dedicated method hitting/health/ready, the parenthetical "(readiness probe)" inhealth_check's docstring is misleading. Consider dropping it or clarifying that/healthis the overall/aggregate status endpoint.Also applies to: 816-828
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android_sms_gateway/client.py` around lines 393 - 405, Update the docstring for health_check to remove or correct the misleading parenthetical "(readiness probe)"—clarify that health_check() calls the aggregate /health endpoint (overall system status) and that the dedicated readiness_check() hits /health/ready for readiness; similarly, find the other duplicate docstring (the same wording around the readiness probe) and apply the same clarification so both health_check() and readiness_check() docstrings accurately describe their distinct endpoints and purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android_sms_gateway/domain.py`:
- Around line 629-630: The annotated type for the attribute release_id is
currently t.Optional[int] but the JSON uses a string; update the type annotation
to t.Optional[str] and any related dataclass/ model field declaration
(release_id) so it accepts string values; also audit and update the
deserialization flow (e.g., from_dict or the class constructor where release_id
is assigned) to stop converting/expecting int and treat the raw JSON string as
the canonical value, and update any type hints/usages that assumed an int to use
str instead.
- Around line 543-584: DeviceSettings.from_dict currently forwards raw dicts
into Settings* constructors which leaves enum-typed fields as plain strings;
update deserialization to coerce those strings back to their Enum members either
by adding/from enhancing a from_dict classmethod on each Settings class (e.g.,
SettingsMessages.from_dict, SettingsLogs.from_dict, SettingsGateway.from_dict,
SettingsEncryption.from_dict, SettingsPing.from_dict,
SettingsWebhooks.from_dict) or by converting fields inline in
DeviceSettings.from_dict before constructing (convert
payload["messages"]["limit_period"] -> LimitPeriod(<value>),
payload["messages"]["processing_order"] -> MessagesProcessingOrder(<value>),
payload["messages"]["sim_selection_mode"] -> SimSelectionMode(<value>), etc.);
ensure every enum field referenced elsewhere (LimitPeriod,
MessagesProcessingOrder, SimSelectionMode, etc.) is converted so asdict() later
will see real Enum instances, not strings.
---
Duplicate comments:
In `@android_sms_gateway/client.py`:
- Around line 911-916: The async method refresh_token is calling self.http.post
without the required payload argument, which causes a TypeError at runtime;
update the call in refresh_token so the await self.http.post(...) includes
payload={} (keeping headers and URL unchanged) before passing the response into
domain.TokenResponse.from_dict; reference the async refresh_token method,
AsyncHttpClient.post requirement, and domain.TokenResponse.from_dict to locate
and fix the call.
- Around line 609-617: The async get_messages docstring is outdated: update the
Args section to match the function signature by replacing the generic `request`
description with entries for `query: MessagesQueryFilter` and `pagination:
QueryPagination`, briefly describing each (query filters and pagination params)
and keep the Returns description as-is; locate the docstring inside the async
def get_messages(...) function and adjust only the parameter names/descriptions
to match the signature.
---
Nitpick comments:
In `@android_sms_gateway/client.py`:
- Line 160: The URL construction currently appends "?{urlencode(params)}"
unconditionally which yields a trailing '?' when params is empty; update the
callers in client.py (methods send, get_messages, get_logs for both sync and
async clients) to build the query string first (qs = urlencode(params)) and then
append it only if non-empty (e.g., url = f"{self.base_url}/messages" + (f"?{qs}"
if qs else "")), replacing the direct
f"{self.base_url}/messages?{urlencode(params)}" usage at the affected spots.
- Around line 393-405: Update the docstring for health_check to remove or
correct the misleading parenthetical "(readiness probe)"—clarify that
health_check() calls the aggregate /health endpoint (overall system status) and
that the dedicated readiness_check() hits /health/ready for readiness;
similarly, find the other duplicate docstring (the same wording around the
readiness probe) and apply the same clarification so both health_check() and
readiness_check() docstrings accurately describe their distinct endpoints and
purpose.
In `@android_sms_gateway/domain.py`:
- Around line 668-691: The LogEntry.from_dict uses
_parse_iso(payload["createdAt"]) which can return None but LogEntry.created_at
is typed as datetime.datetime; fix by either (A) making the dataclass field
LogEntry.created_at Optional[datetime.datetime] and accept None from _parse_iso,
or (B) enforce a non-nullable parse inside from_dict (e.g., parse
payload["createdAt"] with datetime.fromisoformat after replacing "Z" with
"+00:00" and raise/handle if it's missing) so that created_at remains a
datetime; update the LogEntry.created_at annotation and any downstream code
accordingly and reference the from_dict method and _parse_iso function when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d53ff7fd-f69a-4bfa-88c4-2b6321689d70
📒 Files selected for processing (3)
README.mdandroid_sms_gateway/client.pyandroid_sms_gateway/domain.py
✅ Files skipped from review due to trivial changes (1)
- README.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
android_sms_gateway/domain.py (2)
764-794: Inconsistent datetime handling between query filter and export request.
MessagesExportRequest.since/untilare typeddatetime.datetimeand serialized via.isoformat(), whileMessagesQueryFilter.from_/toare plainOptional[str]with a comment "RFC3339 format" — pushing formatting responsibility onto callers and inviting malformed inputs. Consider aligning the types so both acceptdatetime.datetimeand serialize consistently.♻️ Proposed change
- from_: t.Optional[str] = None + from_: t.Optional[datetime.datetime] = None """Start date in RFC3339 format.""" - to: t.Optional[str] = None + to: t.Optional[datetime.datetime] = None """End date in RFC3339 format.""" @@ - if self.from_ is not None: - params["from"] = self.from_ - if self.to is not None: - params["to"] = self.to + if self.from_ is not None: + params["from"] = self.from_.isoformat() + if self.to is not None: + params["to"] = self.to.isoformat()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android_sms_gateway/domain.py` around lines 764 - 794, Change MessagesQueryFilter.from_ and .to from Optional[str] to Optional[datetime.datetime] and update their docstrings to reflect datetime inputs; in MessagesQueryFilter.asdict(), when from_ or to is not None call .isoformat() to serialize to RFC3339 strings (mirroring MessagesExportRequest.since/until behavior); ensure datetime is imported (datetime.datetime) and keep the output keys ("from", "to") the same while leaving device_id/state handling unchanged.
722-731: Reuse_parse_isohelper for consistency.
LogEntry.from_dictduplicates the ISO-parsing logic now encapsulated in_parse_iso(Lines 19–30). Sincecreated_atis required, unwrap the helper’s result with a non-null assertion or a guard — but collapsing the duplication keeps the parsing rules (e.g., trailingZhandling) in one place.♻️ Proposed change
return cls( id=payload["id"], - created_at=datetime.datetime.fromisoformat( - payload["createdAt"].replace("Z", "+00:00") - ), + created_at=_parse_iso(payload["createdAt"]), message=payload["message"],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android_sms_gateway/domain.py` around lines 722 - 731, LogEntry.from_dict duplicates ISO parsing; replace the inline datetime.fromisoformat logic with a call to the existing _parse_iso helper and handle the required created_at value by unwrapping or guarding its result (e.g., call _parse_iso(payload["createdAt"]) and assert/raise if it returns None) so parsing rules (like trailing "Z" handling) are centralized in _parse_iso while still ensuring created_at is non-null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android_sms_gateway/domain.py`:
- Around line 586-608: DeviceSettings.from_dict currently unpacks payload
sub-dicts directly into SettingsGateway, SettingsEncryption, SettingsLogs,
SettingsPing, and SettingsWebhooks which will raise TypeError if the server
sends unknown keys; add a safe deserialization step by either (A) implementing a
from_dict on each Settings* class (mirroring SettingsMessages.from_dict) that
explicitly pulls known fields via payload.get(...) or (B) filter unknown keys
before calling the constructors using dataclasses.fields to build an allowed-key
set (e.g. _only_known(cls, data) -> {k:v for k,v in data.items() if k in
allowed}) and then call SettingsGateway(**_only_known(SettingsGateway,
payload["gateway"])) (and likewise for SettingsEncryption, SettingsLogs,
SettingsPing, SettingsWebhooks).
---
Nitpick comments:
In `@android_sms_gateway/domain.py`:
- Around line 764-794: Change MessagesQueryFilter.from_ and .to from
Optional[str] to Optional[datetime.datetime] and update their docstrings to
reflect datetime inputs; in MessagesQueryFilter.asdict(), when from_ or to is
not None call .isoformat() to serialize to RFC3339 strings (mirroring
MessagesExportRequest.since/until behavior); ensure datetime is imported
(datetime.datetime) and keep the output keys ("from", "to") the same while
leaving device_id/state handling unchanged.
- Around line 722-731: LogEntry.from_dict duplicates ISO parsing; replace the
inline datetime.fromisoformat logic with a call to the existing _parse_iso
helper and handle the required created_at value by unwrapping or guarding its
result (e.g., call _parse_iso(payload["createdAt"]) and assert/raise if it
returns None) so parsing rules (like trailing "Z" handling) are centralized in
_parse_iso while still ensuring created_at is non-null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8e7a2bf-9437-426a-852e-f8a6a16b7ad7
📒 Files selected for processing (2)
android_sms_gateway/client.pyandroid_sms_gateway/domain.py
🚧 Files skipped from review as they are similar to previous changes (1)
- android_sms_gateway/client.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
android_sms_gateway/domain.py (1)
701-702:⚠️ Potential issue | 🟡 Minor
release_idshould bet.Optional[str], nott.Optional[int].Per the IETF Health Check Response Format (draft-inadarei-api-health-check-06),
releaseIdis a string identifier (e.g.,"1.2.2").from_dictpasses the raw value through without coercion, so the annotation is inconsistent with actual runtime values.🛠️ Proposed fix
- release_id: t.Optional[int] = None + release_id: t.Optional[str] = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android_sms_gateway/domain.py` around lines 701 - 702, The field release_id is annotated as t.Optional[int] but the health-check spec and runtime values are strings (e.g. "1.2.2"); change the type annotation to t.Optional[str] and update the docstring to reflect it's a string identifier, and if any deserialization logic in from_dict currently assumes int, adjust it to accept/return the raw string (or coerce via str(value)) so the runtime value matches the new annotation; update references to release_id and from_dict accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android_sms_gateway/domain.py`:
- Around line 740-763: LogEntry.created_at is typed as datetime.datetime but you
call _parse_iso(...) which returns Optional[datetime.datetime]; adjust by either
changing the field annotation to Optional[datetime.datetime] or, preferably,
make from_dict validate/assert the parse result: call created_at =
_parse_iso(payload["createdAt"]), if created_at is None raise a ValueError (or
assert) before passing it into cls(...), keeping the rest of from_dict unchanged
and referencing LogEntry.created_at, LogEntry.from_dict, and _parse_iso to
locate the code to modify.
- Around line 390-408: SettingsGateway.from_dict currently reads camelCase keys;
update it to read snake_case keys to match asdict and other Settings* classes.
Change SettingsGateway.from_dict to pull payload.get("cloud_url"),
payload.get("private_token"), and payload.get("notification_channel") so the
constructor call (in SettingsGateway.from_dict) populates cloud_url,
private_token, and notification_channel consistently with SettingsGateway.asdict
and with other Settings classes (so DeviceSettings.from_dict(...).gateway
round-trips correctly).
---
Duplicate comments:
In `@android_sms_gateway/domain.py`:
- Around line 701-702: The field release_id is annotated as t.Optional[int] but
the health-check spec and runtime values are strings (e.g. "1.2.2"); change the
type annotation to t.Optional[str] and update the docstring to reflect it's a
string identifier, and if any deserialization logic in from_dict currently
assumes int, adjust it to accept/return the raw string (or coerce via
str(value)) so the runtime value matches the new annotation; update references
to release_id and from_dict accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88f8305e-9048-4081-b9a9-4cc18ee13314
📒 Files selected for processing (1)
android_sms_gateway/domain.py
capcom6
force-pushed
the
client/add-new-methods
branch
from
0e606f0 to
049e998
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation