Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Email Logs API: new resource and base API, Pydantic models and filter builders, client integration and exports, example script, unit tests, README/CHANGELOG updates, and pyproject metadata adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MailtrapClient
participant Base as EmailLogsBaseApi
participant API as EmailLogsApi
participant HTTP as HttpClient
participant Server as Mailtrap Email Logs API
Client->>Base: client.email_logs_api
Base-->>Client: EmailLogsApi(client, account_id)
Client->>API: get_list(filters, search_after)
API->>HTTP: GET /accounts/{account_id}/email_logs?...
HTTP->>Server: HTTP GET request
Server-->>HTTP: 200 OK { messages, total_count, next_page_cursor }
HTTP-->>API: raw JSON response
API->>API: EmailLogMessage.from_api(...) for each message
API-->>Client: EmailLogsListResponse(messages, total_count, next_page_cursor)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview 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: 3
🧹 Nitpick comments (1)
tests/unit/api/email_logs/test_email_logs.py (1)
96-117: Consider verifying that filter parameters are actually sent in the request.The test mocks the response and calls the API with filters, but doesn't assert that the request actually contains the expected filter query parameters. The
responseslibrary supports request matching or inspection viaresponses.calls.This is optional, but would strengthen the test by confirming the filters are properly serialized and sent.
💡 Example: Assert request parameters
# After the API call, verify the request: assert len(responses.calls) == 1 request_url = responses.calls[0].request.url assert "filters%5Bsent_after%5D" in request_url # URL-encoded filters[sent_after] assert "search_after=b2c3" in request_url🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api/email_logs/test_email_logs.py` around lines 96 - 117, Enhance the test_get_list_with_filters_and_search_after test to assert the outgoing HTTP request includes the serialized filter and search_after parameters: after calling email_logs_api.get_list(...) inspect responses.calls (ensure len == 1) and examine responses.calls[0].request.url to assert it contains the URL-encoded filters keys (e.g., filters[sent_after], filters[sent_before], filters[to]) and the search_after=b2c3 parameter so the EmailLogsApi.get_list serialization is verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 27: Fix the typo in the changelog entry that currently reads
"releated"—open CHANGELOG.md and replace the word "releated" with "related" in
the line containing "Fix issue `#26`: Add MessagesApi, releated models, examples,
tests by `@Ihor-Bilous` in https://github.com/mailtrap/mailtrap-python/pull/43" so
the sentence reads "related".
In `@mailtrap/client.py`:
- Around line 106-112: The email_logs_api property is calling
_validate_account_id() which raises a hardcoded "account_id is required for
Testing API" message; change _validate_account_id to accept an optional api_name
parameter (or add a new helper like _validate_account_id_for(api_name: str)) and
use that value in the raised error so callers can pass "Email Logs API" from the
email_logs_api property; update email_logs_api to call
_validate_account_id("Email Logs API") (and update any other callers to pass
appropriate names or rely on the default) so the remediation hint is correct.
In `@mailtrap/models/email_logs.py`:
- Around line 59-76: MessageEvent.details uses a bare Union so Pydantic v2 can
mis-select between similar detail types; update the models so selection is
deterministic by either 1) adding an event_type: Literal[...] field to each
detail model (e.g., EventDetailsOpen, EventDetailsUnsubscribe,
EventDetailsDelivery, EventDetailsBounce, etc.) and annotate
MessageEvent.details with Field(discriminator='event_type') so Pydantic can
discriminate, or 2) change MessageEvent.from_api() to inspect the top-level
MessageEvent.event_type and explicitly instantiate the correct EventDetails*
class before constructing MessageEvent; reference MessageEvent, details,
event_type, the EventDetails* classes, and from_api() when making the change.
---
Nitpick comments:
In `@tests/unit/api/email_logs/test_email_logs.py`:
- Around line 96-117: Enhance the test_get_list_with_filters_and_search_after
test to assert the outgoing HTTP request includes the serialized filter and
search_after parameters: after calling email_logs_api.get_list(...) inspect
responses.calls (ensure len == 1) and examine responses.calls[0].request.url to
assert it contains the URL-encoded filters keys (e.g., filters[sent_after],
filters[sent_before], filters[to]) and the search_after=b2c3 parameter so the
EmailLogsApi.get_list serialization is verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4957264-78fb-44a0-8a02-edd5d6a66239
📒 Files selected for processing (12)
CHANGELOG.mdREADME.mdexamples/email_logs/email_logs.pymailtrap/__init__.pymailtrap/api/email_logs.pymailtrap/api/resources/email_logs.pymailtrap/client.pymailtrap/models/email_logs.pypyproject.tomltests/unit/api/email_logs/test_email_logs.pytests/unit/models/test_email_log_models.pytests/unit/test_client.py
mklocek
force-pushed
the
email-logs-api
branch
4 times, most recently
from
6cf1ce7 to
655c6f5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mailtrap/api/resources/email_logs.py (1)
1-1: Minor: Replace EN DASH with HYPHEN-MINUS in docstring.The docstring uses an EN DASH (
–) instead of a standard HYPHEN-MINUS (-). While this doesn't affect functionality, it's flagged by static analysis tools.📝 Suggested fix
-"""Email Logs API resource – list and get email sending logs.""" +"""Email Logs API resource - list and get email sending logs."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mailtrap/api/resources/email_logs.py` at line 1, The module docstring in email_logs.py uses an EN DASH (–) instead of a HYPHEN-MINUS (-); open the top-level docstring (the triple-quoted string at the top of the module in email_logs.py) and replace the EN DASH character with a standard hyphen-minus so the string reads "Email Logs API resource - list and get email sending logs." to satisfy static analysis.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mailtrap/api/resources/email_logs.py`:
- Line 1: The module docstring in email_logs.py uses an EN DASH (–) instead of a
HYPHEN-MINUS (-); open the top-level docstring (the triple-quoted string at the
top of the module in email_logs.py) and replace the EN DASH character with a
standard hyphen-minus so the string reads "Email Logs API resource - list and
get email sending logs." to satisfy static analysis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d286002-7524-4f44-9c4c-5e466e9144b5
📒 Files selected for processing (9)
README.mdexamples/email_logs/email_logs.pymailtrap/__init__.pymailtrap/api/email_logs.pymailtrap/api/resources/email_logs.pymailtrap/client.pymailtrap/models/email_logs.pytests/unit/api/email_logs/test_email_logs.pytests/unit/models/test_email_log_models.py
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- tests/unit/models/test_email_log_models.py
- tests/unit/api/email_logs/test_email_logs.py
piobeny
approved these changes
Mar 18, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
CHANGELOG.md (1)
27-27:⚠️ Potential issue | 🟡 MinorFix changelog typo on Line 27 (
releated→related).This typo is still present in the MessagesApi entry.
✏️ Proposed fix
-- Fix issue `#26`: Add MessagesApi, releated models, examples, tests by `@Ihor-Bilous` in https://github.com/mailtrap/mailtrap-python/pull/43 +- Fix issue `#26`: Add MessagesApi, related models, examples, tests by `@Ihor-Bilous` in https://github.com/mailtrap/mailtrap-python/pull/43🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 27, Typo in the CHANGELOG entry for "Fix issue `#26`: Add MessagesApi, releated models, examples, tests by `@Ihor-Bilous` in https://github.com/mailtrap/mailtrap-python/pull/43" — update the word "releated" to "related" in the MessagesApi changelog line so it reads "related models, examples, tests" (locate the exact string "MessagesApi" or the quoted changelog line to apply the change).
🧹 Nitpick comments (2)
examples/email_logs/email_logs.py (1)
13-17: Prefer environment variables over editable credential constants.At Line 13 and Line 14, this pattern can lead to accidental commits of real secrets after local edits. Use environment variables and fail fast when missing.
🔐 Suggested refactor
+import os + -API_TOKEN = "YOUR_API_TOKEN" -ACCOUNT_ID = "YOUR_ACCOUNT_ID" +API_TOKEN = os.getenv("MAILTRAP_API_TOKEN") +ACCOUNT_ID = os.getenv("MAILTRAP_ACCOUNT_ID") + +if not API_TOKEN or not ACCOUNT_ID: + raise RuntimeError( + "Set MAILTRAP_API_TOKEN and MAILTRAP_ACCOUNT_ID before running this example." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/email_logs/email_logs.py` around lines 13 - 17, Replace the hard-coded API_TOKEN and ACCOUNT_ID constants with environment-based configuration: read os.environ["MAILTRAP_API_TOKEN"] and os.environ["MAILTRAP_ACCOUNT_ID"] (or use os.getenv with explicit checks) before creating the MailtrapClient; if either value is missing, raise a clear exception or exit early so the program fails fast. Update the code around the API_TOKEN/API_ACCOUNT_ID declarations and the client = mt.MailtrapClient(...) instantiation to use the env vars and validate them prior to using email_logs_api.tests/unit/models/test_email_log_models.py (1)
46-187: Reduce duplicated message payload setup across tests.The repeated base payload makes updates error-prone. Consider a small helper/fixture that returns a default message dict and accepts overrides per test.
♻️ Suggested refactor
+def _base_message_data(**overrides: Any) -> dict[str, Any]: + base: dict[str, Any] = { + "message_id": "a1b2c3d4-e5f6-7890-abcd-ef1234567890", + "status": "delivered", + "subject": "Welcome", + "from": "sender@example.com", + "to": "recipient@example.com", + "sent_at": "2025-01-15T10:30:00Z", + "client_ip": None, + "category": None, + "custom_variables": {}, + "sending_stream": "transactional", + "sending_domain_id": 3938, + "template_id": None, + "template_variables": {}, + "opens_count": 0, + "clicks_count": 0, + "raw_message_url": None, + "events": [], + } + base.update(overrides) + return base🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/test_email_log_models.py` around lines 46 - 187, Create a shared factory/fixture that returns the common base payload and allow per-test overrides instead of repeating the dict literal in each test; implement a helper function (e.g., default_email_log_data or a pytest fixture default_email_log_data) and use it in tests before calling EmailLogMessage.from_api, merging specific fields (like "events", "raw_message_url", "clicks_count") to customize each test case so tests such as test_from_api_parses_events_with_deterministic_details_type, test_from_api_parses_click_and_bounce_events, and test_from_api_parses_delivery_event reuse the same baseline payload and only supply the differing keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mailtrap/api/resources/email_logs.py`:
- Line 1: The module docstring at the top of the file contains an en dash (–);
replace it with a standard hyphen (-) so the docstring reads "Email Logs API
resource - list and get email sending logs." Update the module-level string (the
top-of-file docstring) accordingly to satisfy Ruff RUF002.
- Around line 31-37: Guard against non-dict responses from HttpClient.get() in
get_list() and get_by_id(): ensure the value returned from
self._client.get(self._api_path(), ...) is a dict before accessing keys or
passing to EmailLogMessage.from_api; if it's not a dict, convert it to an empty
dict or raise a clear SDK error, then proceed to build EmailLogsListResponse
(using response.get(...)) and to call EmailLogMessage.from_api only for dict
items; reference the methods get_list(), get_by_id(), EmailLogMessage.from_api,
EmailLogsListResponse and the call site self._client.get(...) when making the
change.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Line 27: Typo in the CHANGELOG entry for "Fix issue `#26`: Add MessagesApi,
releated models, examples, tests by `@Ihor-Bilous` in
https://github.com/mailtrap/mailtrap-python/pull/43" — update the word
"releated" to "related" in the MessagesApi changelog line so it reads "related
models, examples, tests" (locate the exact string "MessagesApi" or the quoted
changelog line to apply the change).
---
Nitpick comments:
In `@examples/email_logs/email_logs.py`:
- Around line 13-17: Replace the hard-coded API_TOKEN and ACCOUNT_ID constants
with environment-based configuration: read os.environ["MAILTRAP_API_TOKEN"] and
os.environ["MAILTRAP_ACCOUNT_ID"] (or use os.getenv with explicit checks) before
creating the MailtrapClient; if either value is missing, raise a clear exception
or exit early so the program fails fast. Update the code around the
API_TOKEN/API_ACCOUNT_ID declarations and the client = mt.MailtrapClient(...)
instantiation to use the env vars and validate them prior to using
email_logs_api.
In `@tests/unit/models/test_email_log_models.py`:
- Around line 46-187: Create a shared factory/fixture that returns the common
base payload and allow per-test overrides instead of repeating the dict literal
in each test; implement a helper function (e.g., default_email_log_data or a
pytest fixture default_email_log_data) and use it in tests before calling
EmailLogMessage.from_api, merging specific fields (like "events",
"raw_message_url", "clicks_count") to customize each test case so tests such as
test_from_api_parses_events_with_deterministic_details_type,
test_from_api_parses_click_and_bounce_events, and
test_from_api_parses_delivery_event reuse the same baseline payload and only
supply the differing keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3151adc8-6531-49ce-a123-9b71917b170a
📒 Files selected for processing (11)
CHANGELOG.mdREADME.mdexamples/email_logs/email_logs.pymailtrap/__init__.pymailtrap/api/email_logs.pymailtrap/api/resources/email_logs.pymailtrap/client.pymailtrap/models/email_logs.pypyproject.tomltests/unit/api/email_logs/test_email_logs.pytests/unit/models/test_email_log_models.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/api/email_logs/test_email_logs.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
mailtrap/api/resources/email_logs.py (2)
1-1:⚠️ Potential issue | 🟡 MinorReplace the EN DASH in the module docstring.
Line 1 still uses
–, which triggers RuffRUF002.Proposed fix
-"""Email Logs API resource – list and get email sending logs.""" +"""Email Logs API resource - list and get email sending logs."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mailtrap/api/resources/email_logs.py` at line 1, The module docstring in email_logs.py uses an en dash character (–) which triggers Ruff RUF002; replace it with a standard ASCII hyphen-minus (-) in the module docstring string at the top of the file so the comment reads "Email Logs API resource - list and get email sending logs." and re-run linting to confirm the RUF002 warning is resolved.
31-37:⚠️ Potential issue | 🟠 MajorGuard response shape before key access/model parsing.
Line 31 and Line 41 treat
HttpClient.get()output as a dict unconditionally. If a non-dict payload is returned, this will fail with unclear runtime errors.Proposed fix
-from typing import Optional +from typing import Any +from typing import Optional @@ class EmailLogsApi: + def _expect_dict_response( + self, response: Any, operation: str + ) -> dict[str, Any]: + if not isinstance(response, dict): + raise TypeError( + f"Unexpected response type for {operation}: {type(response).__name__}" + ) + return response + @@ - response = self._client.get(self._api_path(), params=params or None) - messages = [EmailLogMessage.from_api(msg) for msg in response.get("messages", [])] + response = self._expect_dict_response( + self._client.get(self._api_path(), params=params or None), + "email logs list", + ) + messages = [ + EmailLogMessage.from_api(msg) + for msg in response.get("messages", []) + if isinstance(msg, dict) + ] @@ - response = self._client.get(self._api_path(sending_message_id)) + response = self._expect_dict_response( + self._client.get(self._api_path(sending_message_id)), + "email log by id", + ) return EmailLogMessage.from_api(response)Also applies to: 41-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mailtrap/api/resources/email_logs.py` around lines 31 - 37, The code assumes self._client.get(...) returns a dict and directly calls response.get(...) and iterates response["messages"]; add explicit shape guards: verify response is a dict (isinstance(response, dict)) before accessing keys in the method that builds EmailLogsListResponse and in the block that constructs EmailLogMessage.from_api for individual messages, and if not, either coerce to a safe default (e.g., response = {}) or raise a clear ValueError/TypeError with context; also ensure response.get("messages") is a list before iterating and handle non-list by treating it as an empty list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/email_logs/email_logs.py`:
- Around line 3-17: Replace hardcoded API_TOKEN and ACCOUNT_ID with
environment-variable reads and validate them before creating the MailtrapClient:
read os.environ["MAILTRAP_API_TOKEN"] and os.environ["MAILTRAP_ACCOUNT_ID"] (or
use os.getenv with explicit check), raise or log a clear error if missing, then
pass those values into MailtrapClient(token=..., account_id=...). Update
references to the API_TOKEN and ACCOUNT_ID symbols and ensure the client
initialization (client = mt.MailtrapClient(...)) only runs after the env vars
are validated.
---
Duplicate comments:
In `@mailtrap/api/resources/email_logs.py`:
- Line 1: The module docstring in email_logs.py uses an en dash character (–)
which triggers Ruff RUF002; replace it with a standard ASCII hyphen-minus (-) in
the module docstring string at the top of the file so the comment reads "Email
Logs API resource - list and get email sending logs." and re-run linting to
confirm the RUF002 warning is resolved.
- Around line 31-37: The code assumes self._client.get(...) returns a dict and
directly calls response.get(...) and iterates response["messages"]; add explicit
shape guards: verify response is a dict (isinstance(response, dict)) before
accessing keys in the method that builds EmailLogsListResponse and in the block
that constructs EmailLogMessage.from_api for individual messages, and if not,
either coerce to a safe default (e.g., response = {}) or raise a clear
ValueError/TypeError with context; also ensure response.get("messages") is a
list before iterating and handle non-list by treating it as an empty list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17326178-5cbd-4ee5-8b2b-57af338a5cec
📒 Files selected for processing (11)
CHANGELOG.mdREADME.mdexamples/email_logs/email_logs.pymailtrap/__init__.pymailtrap/api/email_logs.pymailtrap/api/resources/email_logs.pymailtrap/client.pymailtrap/models/email_logs.pypyproject.tomltests/unit/api/email_logs/test_email_logs.pytests/unit/models/test_email_log_models.py
✅ Files skipped from review due to trivial changes (6)
- CHANGELOG.md
- README.md
- mailtrap/init.py
- pyproject.toml
- tests/unit/models/test_email_log_models.py
- tests/unit/api/email_logs/test_email_logs.py
🚧 Files skipped from review as they are similar to previous changes (1)
- mailtrap/api/email_logs.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
examples/email_logs/email_logs.py (1)
13-14:⚠️ Potential issue | 🟡 MinorUse environment variables instead of hardcoded placeholders.
Hardcoded credential placeholders can trigger secret scanners and encourage users to commit real secrets. Use environment variables with validation instead.
🛡️ Proposed fix
+import os from datetime import datetime from datetime import timedelta from datetime import timezone import mailtrap as mt from mailtrap.models.email_logs import EmailLogsListFilters from mailtrap.models.email_logs import filter_ci_equal from mailtrap.models.email_logs import filter_string_equal from mailtrap.models.email_logs import filter_string_not_empty -API_TOKEN = "YOUR_API_TOKEN" -ACCOUNT_ID = "YOUR_ACCOUNT_ID" +API_TOKEN = os.environ.get("MAILTRAP_API_TOKEN") +ACCOUNT_ID = os.environ.get("MAILTRAP_ACCOUNT_ID") + +if not API_TOKEN or not ACCOUNT_ID: + raise SystemExit( + "Set MAILTRAP_API_TOKEN and MAILTRAP_ACCOUNT_ID environment variables." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/email_logs/email_logs.py` around lines 13 - 14, Replace the hardcoded API_TOKEN and ACCOUNT_ID placeholders with reads from environment variables and validate they exist before use: replace references to the constants API_TOKEN and ACCOUNT_ID with values loaded from os.environ (or an equivalent config loader) and raise a clear error (or exit) if either is missing so callers cannot run with unset credentials; include brief guidance in the error message naming the required env var names.
🧹 Nitpick comments (2)
mailtrap/api/resources/email_logs.py (1)
32-33: Response guard implemented with silent fallback.The guard converts non-dict responses to an empty dict, which gracefully returns zero results rather than raising an exception. This is a reasonable trade-off for list endpoints, though it may hide unexpected API behavior. Consider logging a warning when this fallback is triggered to aid debugging.
♻️ Optional: Add warning for unexpected response type
+import logging + +logger = logging.getLogger(__name__) + class EmailLogsApi: ... def get_list( ... ) -> EmailLogsListResponse: ... response = self._client.get(self._api_path(), params=params or None) if not isinstance(response, dict): + logger.warning( + "Email Logs API returned unexpected response type %s for list; " + "defaulting to empty results", + type(response).__name__, + ) response = {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mailtrap/api/resources/email_logs.py` around lines 32 - 33, The code silently replaces a non-dict "response" with {} which hides unexpected API behavior; update the guard around response (the check "if not isinstance(response, dict): response = {}") to log a warning when triggered (include the actual type and a snippet or repr of response) using the module logger or logging.warning before falling back, so callers can trace unexpected response shapes while keeping the empty-dict fallback for list endpoints.mailtrap/models/email_logs.py (1)
79-111: Handle unexpected fields in event details gracefully.The
**dataunpacking will raiseTypeErrorif the API returns additional fields not defined in the dataclass (e.g., if a new field is added in a future API version). Consider filtering to known fields or catching the exception.♻️ Proposed fix to handle extra fields
+import dataclasses + +def _safe_construct(cls, data: dict[str, Any]): + """Construct dataclass, ignoring unknown fields.""" + field_names = {f.name for f in dataclasses.fields(cls)} + return cls(**{k: v for k, v in data.items() if k in field_names}) + def _parse_event_details( event_type: str, data: Optional[dict[str, Any]], ) -> Optional[...]: ... if data is None: return None if event_type == "delivery": - return EventDetailsDelivery(**data) + return _safe_construct(EventDetailsDelivery, data) if event_type == "open": - return EventDetailsOpen(**data) + return _safe_construct(EventDetailsOpen, data) # ... apply to all other cases🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mailtrap/models/email_logs.py` around lines 79 - 111, The _parse_event_details function currently unpacks raw data with **data into dataclass constructors (EventDetailsDelivery, EventDetailsOpen, EventDetailsClick, EventDetailsBounce, EventDetailsSpam, EventDetailsUnsubscribe, EventDetailsReject), which will raise TypeError if the API returns unexpected fields; update each branch to filter the incoming data to only the dataclass's defined fields before constructing the instance (e.g., use dataclasses.fields(...) or the dataclass's __annotations__ to build an allowed-keys set and do {k:v for k,v in data.items() if k in allowed}), and optionally wrap the constructor in a try/except TypeError to fall back to returning the original dict when parsing still fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@examples/email_logs/email_logs.py`:
- Around line 13-14: Replace the hardcoded API_TOKEN and ACCOUNT_ID placeholders
with reads from environment variables and validate they exist before use:
replace references to the constants API_TOKEN and ACCOUNT_ID with values loaded
from os.environ (or an equivalent config loader) and raise a clear error (or
exit) if either is missing so callers cannot run with unset credentials; include
brief guidance in the error message naming the required env var names.
---
Nitpick comments:
In `@mailtrap/api/resources/email_logs.py`:
- Around line 32-33: The code silently replaces a non-dict "response" with {}
which hides unexpected API behavior; update the guard around response (the check
"if not isinstance(response, dict): response = {}") to log a warning when
triggered (include the actual type and a snippet or repr of response) using the
module logger or logging.warning before falling back, so callers can trace
unexpected response shapes while keeping the empty-dict fallback for list
endpoints.
In `@mailtrap/models/email_logs.py`:
- Around line 79-111: The _parse_event_details function currently unpacks raw
data with **data into dataclass constructors (EventDetailsDelivery,
EventDetailsOpen, EventDetailsClick, EventDetailsBounce, EventDetailsSpam,
EventDetailsUnsubscribe, EventDetailsReject), which will raise TypeError if the
API returns unexpected fields; update each branch to filter the incoming data to
only the dataclass's defined fields before constructing the instance (e.g., use
dataclasses.fields(...) or the dataclass's __annotations__ to build an
allowed-keys set and do {k:v for k,v in data.items() if k in allowed}), and
optionally wrap the constructor in a try/except TypeError to fall back to
returning the original dict when parsing still fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c547d75f-2f34-4a2d-a24f-8dc20cae6172
📒 Files selected for processing (10)
CHANGELOG.mdREADME.mdexamples/email_logs/email_logs.pymailtrap/__init__.pymailtrap/api/email_logs.pymailtrap/api/resources/email_logs.pymailtrap/client.pymailtrap/models/email_logs.pytests/unit/api/email_logs/test_email_logs.pytests/unit/models/test_email_log_models.py
✅ Files skipped from review due to trivial changes (5)
- CHANGELOG.md
- mailtrap/api/email_logs.py
- README.md
- mailtrap/init.py
- tests/unit/models/test_email_log_models.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/api/email_logs/test_email_logs.py
|
|
||
|
|
||
| @dataclass | ||
| class EventDetailsDelivery: |
Contributor
There was a problem hiding this comment.
I'm not a pythonist - but this class approach seems a bit heavy for data objects. Is it common practice?
Contributor
Author
There was a problem hiding this comment.
At least it's common for us (consistent with how we handle other API responses): https://github.com/mailtrap/mailtrap-python/blob/main/mailtrap/models/sending_domains.py. IMO that's better than returning an unspecified dict and forcing users to check API docs to know what are the expected event fields.
Open
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
3 participants
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.
Changes
Summary by CodeRabbit
New Features
Documentation
Tests
Chores