Issue 25 #33
Issue 25 #33
Conversation
WalkthroughRefactors sending flow: introduces a SendingApi and HttpClient usage, moves mail models to pydantic dataclasses under Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MailtrapClient
participant SendingApi
participant HttpClient
participant MailtrapAPI
User->>MailtrapClient: send(mail)
MailtrapClient->>SendingApi: send(mail)
alt inbox-scoped
SendingApi->>HttpClient: POST https://<SANDBOX_HOST>/api/send/{inbox_id}\njson=mail.api_data
else global
SendingApi->>HttpClient: POST https://<SENDING_HOST>/api/send\njson=mail.api_data
end
HttpClient->>MailtrapAPI: HTTP POST
MailtrapAPI-->>HttpClient: JSON response
HttpClient-->>SendingApi: response dict
SendingApi-->>MailtrapClient: SendingMailResponse
MailtrapClient-->>User: SEND_ENDPOINT_RESPONSE (serialized)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (21)
mailtrap/config.py (1)
2-4: Host constants look good; add typing Final for immutability and intentThese hosts are likely to be true constants. Annotating them as Final improves readability and tooling/static analysis.
Apply this diff within the selected lines:
-BULK_HOST = "bulk.api.mailtrap.io" -SANDBOX_HOST = "sandbox.api.mailtrap.io" -SENDING_HOST = "send.api.mailtrap.io" +BULK_HOST: Final[str] = "bulk.api.mailtrap.io" +SANDBOX_HOST: Final[str] = "sandbox.api.mailtrap.io" +SENDING_HOST: Final[str] = "send.api.mailtrap.io"And add this import at the top of the file (outside the selected range):
from typing import Finaltests/unit/mail/test_mail.py (1)
6-6: Rename test class to reflect subject under testThis class tests Mail, not Attachment. Renaming improves clarity for test reports and future maintenance.
Apply this diff:
-class TestAttachment: +class TestMail:tests/unit/mail/test_from_template.py (1)
6-6: Rename test class for accuracyThis suite validates MailFromTemplate. Rename for clearer test intent and discoverability.
Apply this diff:
-class TestAttachment: +class TestMailFromTemplate:mailtrap/models/mail/address.py (1)
1-11: Prefer EmailStr for validated email addressesUsing EmailStr adds lightweight validation and produces clearer API errors on invalid emails, with no changes required at call sites.
-from pydantic.dataclasses import dataclass +from pydantic import EmailStr +from pydantic.dataclasses import dataclass @@ class Address(RequestModel): - email: str + email: EmailStr name: Optional[str] = Nonemailtrap/models/mail/mail.py (3)
9-11: Guard against dataclass field-ordering pitfall (make subclass fields keyword-only)BaseMail defines fields with defaults (e.g., cc, bcc, ...). Adding a required field (subject) in a subclass can trigger "non-default argument follows default argument" in dataclasses. Marking subclass fields kw-only avoids this class of issues across Python versions.
-@dataclass +@dataclass(kw_only=True) class Mail(BaseMail): subject: str = Field(...) # type:ignoreIf BaseMail is already declared with kw_only=True, this change is optional; otherwise it’s a safe guardrail.
1-4: Remove type:ignore by using Annotated with Field for a required attributeThis keeps type-checkers happy without suppressing checks.
-from pydantic import Field +from pydantic import Field +from typing_extensions import Annotated @@ - subject: str = Field(...) # type:ignore + subject: Annotated[str, Field(...)]Also applies to: 11-11
12-14: Optional: enforce that at least one of text or html is providedIf your API requires at least one body variant, enforce it at model level for earlier feedback.
class Mail(BaseMail): subject: str = Field(...) # type:ignore text: Optional[str] = None html: Optional[str] = None category: Optional[str] = None + def __post_init_post_parse__(self) -> None: + if self.text is None and self.html is None: + raise ValueError("At least one of `text` or `html` must be provided")Note: post_init_post_parse is called after Pydantic validation in dataclass mode. If you prefer not to couple domain rules to the model, consider validating in the sending layer instead.
mailtrap/models/mail/from_template.py (1)
10-13: Apply the same kw_only safeguard to avoid dataclass ordering issuesAs with Mail, this subclass adds a required field while BaseMail has defaults; kw_only avoids "non-default follows default" problems.
-@dataclass +@dataclass(kw_only=True) class MailFromTemplate(BaseMail): template_uuid: str = Field(...) # type:ignore template_variables: Optional[dict[str, Any]] = Nonemailtrap/models/common.py (1)
1-5: Micro-optimization: cache TypeAdapter per classAvoids re-creating TypeAdapter on every api_data call, which can add up in hot paths (bulk sends).
from typing import Any from typing import TypeVar from typing import cast from pydantic import TypeAdapter +from functools import lru_cache @@ class RequestModel: @property def api_data(self: T) -> dict[str, Any]: - return cast( - dict[str, Any], - TypeAdapter(type(self)).dump_python(self, by_alias=True, exclude_none=True), - ) + return cast( + dict[str, Any], + _adapter_for(type(self)).dump_python(self, by_alias=True, exclude_none=True), + ) + +@lru_cache(maxsize=None) +def _adapter_for(tp: type[Any]) -> TypeAdapter: + return TypeAdapter(tp)Also applies to: 14-18
tests/unit/test_client.py (1)
52-65: Parametric validation of sending_api selection looks comprehensiveCovers default, bulk, and sandbox flows. One additional test worth considering: verify that MailtrapClient.send delegates to client.sending_api.send for both default and sandbox cases (using a stubbed/mocked SendingApi). This would guard against regressions if the delegation changes.
I can draft a unit test using monkeypatch to stub DefaultSendingApi/SandboxSendingApi.send and assert it’s called with mail.api_data. Want me to add that?
mailtrap/__init__.py (1)
1-12: Explicitly declare public API to satisfy linters and clarify exportsRuff flags all these imports as unused because they are only for re-export. Define
__all__to make the intent explicit and silence F401. This also documents the public surface.Apply this diff to declare the public API:
from .api.sending import SEND_ENDPOINT_RESPONSE from .client import MailtrapClient from .exceptions import APIError from .exceptions import AuthorizationError from .exceptions import ClientConfigurationError from .exceptions import MailtrapError from .models.mail import Address from .models.mail import Attachment from .models.mail import BaseMail from .models.mail import Disposition from .models.mail import Mail from .models.mail import MailFromTemplate + +__all__ = [ + "SEND_ENDPOINT_RESPONSE", + "MailtrapClient", + "MailtrapError", + "ClientConfigurationError", + "APIError", + "AuthorizationError", + "Address", + "Attachment", + "BaseMail", + "Disposition", + "Mail", + "MailFromTemplate", +]mailtrap/models/mail/base.py (1)
12-20: Verify Pydantic dataclass Field usage and alias behaviorUsing
pydantic.Field(...)directly as the default in apydantic.dataclasses.dataclassis unconventional; in v2 the recommended pattern isdataclasses.field(metadata={'pydantic': Field(...)}). Please verify that:
mail.api_dataincludes the key "from" (alias forsender)- None fields are excluded as expected (exclude_none) for all optional attributes
If the alias isn’t being applied, consider switching to either:
dataclasses.field(metadata={'pydantic': Field(serialization_alias="from")})for dataclass fields, or- Pydantic
BaseModel, which works seamlessly withField(...)andfield_serializer.I can provide a concrete refactor for either approach if you prefer.
tests/unit/api/test_sending.py (2)
15-17: Remove unused constants to avoid lint warnings
ACCOUNT_IDandPROJECT_IDare not used. Drop them to keep tests tidy.-ACCOUNT_ID = "321" -PROJECT_ID = 123 INBOX_ID = "456"
120-123: Compare JSON payloads semantically, not by raw bytesStringifying dicts can differ in key ordering and whitespace. Decode and
json.loadsthe request body and compare dictionaries for a more robust assertion.- request = responses.calls[0].request # type: ignore - assert request.body == json.dumps(mail.api_data).encode() + request = responses.calls[0].request # type: ignore + assert json.loads(request.body.decode()) == mail.api_dataexamples/sending.py (2)
3-4: Fix placeholder typos for clarityMinor typos in placeholders.
-API_TOKEN = "<YOU_API_TOKEN>" +API_TOKEN = "<YOUR_API_TOKEN>" INBOX_ID = "<YOUR_INBOX_ID>"
22-22: Fix template UUID placeholder typo- template_uuid="<YOUT_TEMPLATE_UUID>", + template_uuid="<YOUR_TEMPLATE_UUID>",mailtrap/api/sending.py (2)
8-8: Use a TypedDict for the response type instead of a loose dict aliasA TypedDict communicates expected keys clearly and gives better type safety at call sites without losing flexibility.
Apply:
-from typing import Protocol +from typing import Protocol +from typing import TypedDict from typing import Union from typing import cast @@ -SEND_ENDPOINT_RESPONSE = dict[str, Union[bool, list[str]]] +class _SendEndpointResponse(TypedDict, total=False): + success: bool + errors: list[str] + # Add optional keys here if the API returns more, e.g.: + # message_ids: list[str] + +SEND_ENDPOINT_RESPONSE = _SendEndpointResponse
15-23: Small DRY: share the send path assemblyDefault and Sandbox senders only differ by path. A tiny helper reduces duplication and centralizes the endpoint shape.
Example refactor:
class DefaultSendingApi: def __init__(self, client: HttpClient) -> None: self._client = client def send(self, mail: BaseMail) -> SEND_ENDPOINT_RESPONSE: - return cast( - SEND_ENDPOINT_RESPONSE, self._client.post("/api/send", json=mail.api_data) - ) + return cast( + SEND_ENDPOINT_RESPONSE, self._post("/api/send", mail) + ) + def _post(self, path: str, mail: BaseMail) -> SEND_ENDPOINT_RESPONSE: + return cast(SEND_ENDPOINT_RESPONSE, self._client.post(path, json=mail.api_data)) @@ class SandboxSendingApi: def __init__(self, inbox_id: str, client: HttpClient) -> None: self.inbox_id = inbox_id self._client = client def send(self, mail: BaseMail) -> SEND_ENDPOINT_RESPONSE: - return cast( - SEND_ENDPOINT_RESPONSE, - self._client.post(f"/api/send/{self.inbox_id}", json=mail.api_data), - ) + return cast( + SEND_ENDPOINT_RESPONSE, + self._client.post(f"/api/send/{self.inbox_id}", json=mail.api_data), + )Or, derive Sandbox from Default and override just the path.
Also applies to: 25-34
mailtrap/client.py (3)
51-59: Reuse a single HttpClient/session and cache the SendingApiCurrently a new HttpClient (and requests.Session) is created on every access, which loses connection reuse and adds overhead. Cache the constructed SendingApi.
Apply:
@property def sending_api(self) -> SendingApi: - http_client = HttpClient(host=self._sending_api_host, headers=self.headers) - if self.sandbox: - return SandboxSendingApi( - inbox_id=cast(str, self.inbox_id), client=http_client - ) - return DefaultSendingApi(client=http_client) + # Lazily construct and cache to reuse a single Session/connection pool + if hasattr(self, "_sending_api") and self._sending_api is not None: # type: ignore[attr-defined] + return self._sending_api # type: ignore[return-value] + http_client = HttpClient(host=self._sending_api_host, headers=self.headers) + if self.sandbox: + self._sending_api = SandboxSendingApi( # type: ignore[attr-defined] + inbox_id=cast(str, self.inbox_id), client=http_client + ) + else: + self._sending_api = DefaultSendingApi(client=http_client) # type: ignore[attr-defined] + return self._sending_api # type: ignore[return-value]And initialize the cache in init (outside this hunk):
# in __init__ self._sending_api: Optional[SendingApi] = NoneNote: If you intend to support mutating config (sandbox/inbox_id/api_host) after init, add invalidation logic.
60-61: Ensure callers receive the response; fix example to return the valuesend() correctly returns the API response. The example in examples/sending.py doesn’t return it.
Proposed example fix (outside this file):
def send(client: mt.MailtrapClient, mail: mt.BaseMail) -> mt.SEND_ENDPOINT_RESPONSE: return client.send(mail)
65-71: api_port isn’t applied to live HTTPClient requestsHttpClient._url (mailtrap/http.py lines 47–48) always builds URLs as
f"https://{self._host}/{path.lstrip('/')}"so any api_port passed into the client has no effect on real requests (it only applies to deprecated base_url/api_send_url).
Two ways to resolve:
Deprecate api_port in HttpClient.init:
• Add awarnings.warn(..., DeprecationWarning, stacklevel=2)when api_port is provided.
• Update docs to state that live requests ignore custom ports.Respect api_port in HttpClient:
• Change init to accept and store a port parameter (e.g.self._port).
• Update _url to include it, e.g.:def _url(self, path: str) -> str: return f"https://{self._host.rstrip('/')}" f":{self._port}/{path.lstrip('/')}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
examples/sending.py(1 hunks)mailtrap/__init__.py(1 hunks)mailtrap/api/sending.py(1 hunks)mailtrap/client.py(3 hunks)mailtrap/config.py(1 hunks)mailtrap/mail/__init__.py(0 hunks)mailtrap/mail/address.py(0 hunks)mailtrap/mail/attachment.py(0 hunks)mailtrap/mail/base.py(0 hunks)mailtrap/mail/base_entity.py(0 hunks)mailtrap/mail/from_template.py(0 hunks)mailtrap/mail/mail.py(0 hunks)mailtrap/models/common.py(1 hunks)mailtrap/models/mail/__init__.py(1 hunks)mailtrap/models/mail/address.py(1 hunks)mailtrap/models/mail/attachment.py(1 hunks)mailtrap/models/mail/base.py(1 hunks)mailtrap/models/mail/from_template.py(1 hunks)mailtrap/models/mail/mail.py(1 hunks)tests/unit/api/test_sending.py(1 hunks)tests/unit/mail/test_address.py(1 hunks)tests/unit/mail/test_attachment.py(1 hunks)tests/unit/mail/test_from_template.py(1 hunks)tests/unit/mail/test_mail.py(1 hunks)tests/unit/test_client.py(2 hunks)
💤 Files with no reviewable changes (7)
- mailtrap/mail/from_template.py
- mailtrap/mail/base_entity.py
- mailtrap/mail/init.py
- mailtrap/mail/base.py
- mailtrap/mail/mail.py
- mailtrap/mail/attachment.py
- mailtrap/mail/address.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#31
File: mailtrap/config.py:1-1
Timestamp: 2025-08-12T23:07:25.653Z
Learning: In Mailtrap's API architecture, Testing API resources (Projects, Inboxes, etc.) use the main "mailtrap.io" host, while only email sending functionality uses "sandbox.api.mailtrap.io" as the host.
📚 Learning: 2025-08-12T23:07:25.653Z
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#31
File: mailtrap/config.py:1-1
Timestamp: 2025-08-12T23:07:25.653Z
Learning: In Mailtrap's API architecture, Testing API resources (Projects, Inboxes, etc.) use the main "mailtrap.io" host, while only email sending functionality uses "sandbox.api.mailtrap.io" as the host.
Applied to files:
mailtrap/config.pymailtrap/api/sending.pytests/unit/api/test_sending.pyexamples/sending.pymailtrap/client.py
🧬 Code Graph Analysis (16)
mailtrap/models/mail/base.py (4)
mailtrap/models/common.py (1)
RequestModel(12-18)mailtrap/models/mail/address.py (1)
Address(9-11)mailtrap/models/mail/attachment.py (1)
Attachment(18-27)mailtrap/client.py (1)
headers(85-92)
tests/unit/mail/test_mail.py (3)
mailtrap/models/mail/address.py (1)
Address(9-11)mailtrap/models/mail/attachment.py (1)
Attachment(18-27)mailtrap/models/mail/mail.py (1)
tests/unit/mail/test_address.py (1)
mailtrap/models/mail/address.py (1)
Address(9-11)
mailtrap/models/mail/address.py (1)
mailtrap/models/common.py (1)
RequestModel(12-18)
mailtrap/models/mail/from_template.py (1)
mailtrap/models/mail/base.py (1)
BaseMail(13-20)
tests/unit/mail/test_from_template.py (3)
mailtrap/models/mail/address.py (1)
Address(9-11)mailtrap/models/mail/attachment.py (1)
Attachment(18-27)mailtrap/models/mail/from_template.py (1)
MailFromTemplate(11-13)
tests/unit/test_client.py (2)
mailtrap/api/sending.py (2)
DefaultSendingApi(15-22)SandboxSendingApi(25-34)mailtrap/client.py (2)
testing_api(43-49)sending_api(52-58)
tests/unit/mail/test_attachment.py (1)
mailtrap/models/mail/attachment.py (2)
Attachment(18-27)Disposition(12-14)
mailtrap/api/sending.py (5)
mailtrap/http.py (2)
HttpClient(13-92)post(31-33)mailtrap/models/mail/base.py (1)
BaseMail(13-20)examples/sending.py (1)
send(34-35)mailtrap/client.py (1)
send(60-61)mailtrap/models/common.py (1)
api_data(14-18)
mailtrap/models/mail/mail.py (1)
mailtrap/models/mail/base.py (1)
BaseMail(13-20)
mailtrap/models/mail/__init__.py (5)
mailtrap/models/mail/address.py (1)
Address(9-11)mailtrap/models/mail/attachment.py (2)
Attachment(18-27)Disposition(12-14)mailtrap/models/mail/base.py (1)
BaseMail(13-20)mailtrap/models/mail/from_template.py (1)
MailFromTemplate(11-13)mailtrap/models/mail/mail.py (1)
tests/unit/api/test_sending.py (8)
mailtrap/api/sending.py (6)
DefaultSendingApi(15-22)SandboxSendingApi(25-34)SendingApi(11-12)send(12-12)send(19-22)send(30-34)mailtrap/http.py (2)
HttpClient(13-92)post(31-33)mailtrap/models/mail/address.py (1)
Address(9-11)mailtrap/models/mail/mail.py (1)
mailtrap/models/mail/from_template.py (1)
MailFromTemplate(11-13)mailtrap/models/mail/base.py (1)
BaseMail(13-20)mailtrap/exceptions.py (2)
AuthorizationError(18-20)APIError(10-15)mailtrap/models/common.py (1)
api_data(14-18)
mailtrap/__init__.py (7)
mailtrap/client.py (1)
MailtrapClient(19-118)mailtrap/exceptions.py (4)
APIError(10-15)AuthorizationError(18-20)ClientConfigurationError(5-7)MailtrapError(1-2)mailtrap/models/mail/address.py (1)
Address(9-11)mailtrap/models/mail/attachment.py (2)
Attachment(18-27)Disposition(12-14)mailtrap/models/mail/base.py (1)
BaseMail(13-20)mailtrap/models/mail/mail.py (1)
mailtrap/models/mail/from_template.py (1)
MailFromTemplate(11-13)
examples/sending.py (6)
mailtrap/client.py (2)
MailtrapClient(19-118)send(60-61)mailtrap/models/mail/mail.py (1)
mailtrap/models/mail/address.py (1)
Address(9-11)mailtrap/models/mail/from_template.py (1)
MailFromTemplate(11-13)mailtrap/api/sending.py (3)
send(12-12)send(19-22)send(30-34)mailtrap/models/mail/base.py (1)
BaseMail(13-20)
mailtrap/models/mail/attachment.py (1)
mailtrap/models/common.py (1)
RequestModel(12-18)
mailtrap/client.py (6)
mailtrap/api/sending.py (6)
DefaultSendingApi(15-22)SandboxSendingApi(25-34)SendingApi(11-12)send(12-12)send(19-22)send(30-34)mailtrap/api/testing.py (1)
TestingApi(7-17)mailtrap/exceptions.py (1)
ClientConfigurationError(5-7)mailtrap/http.py (1)
HttpClient(13-92)mailtrap/models/mail/base.py (1)
BaseMail(13-20)examples/sending.py (1)
send(34-35)
🪛 Ruff (0.12.2)
mailtrap/__init__.py
1-1: .api.sending.SEND_ENDPOINT_RESPONSE imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
2-2: .client.MailtrapClient imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
3-3: .exceptions.APIError imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
4-4: .exceptions.AuthorizationError imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
5-5: .exceptions.ClientConfigurationError imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
6-6: .exceptions.MailtrapError imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
7-7: .models.mail.Address imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
8-8: .models.mail.Attachment imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
9-9: .models.mail.BaseMail imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
10-10: .models.mail.Disposition imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
11-11: .models.mail.Mail imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
12-12: .models.mail.MailFromTemplate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
🔇 Additional comments (14)
mailtrap/config.py (1)
2-4: Host-to-API mapping verified—no changes needed
- GENERAL_HOST is used for Testing API (Projects, Inboxes, etc.) via HttpClient(GENERAL_HOST).
- SANDBOX_HOST is used for sandbox sending.
- SENDING_HOST is used for production sending.
- BULK_HOST is returned when the client’s
bulkflag is set (bulk sending).- Legacy
base_url/api_send_urlproperties remain only to emit deprecation warnings and are exercised in tests—properly deprecated.tests/unit/mail/test_mail.py (1)
1-3: Import path update aligns with the new models packageThe move to mailtrap.models.mail keeps tests consistent with the refactor. No issues with the entities’ usage.
tests/unit/mail/test_attachment.py (1)
1-2: Import path update looks correctThe imports now point to the refactored models module; the assertions match the serializer behavior (mimetype alias “type”, disposition enum string values).
tests/unit/mail/test_address.py (1)
1-1: Import path update is consistent with the refactorAddress moved under mailtrap.models.mail.address; tests remain valid and focused on api_data behavior.
tests/unit/mail/test_from_template.py (1)
1-3: Model import updates are correctSwitching to mailtrap.models.mail for Address, Attachment, and MailFromTemplate aligns with the new module structure.
mailtrap/models/mail/address.py (1)
8-11: LGTM: Address model is clean and consistent with RequestModel serializationSimple, minimal, and aligns with the new RequestModel serialization strategy. No concerns.
mailtrap/models/common.py (1)
11-19: Solid, uniform serialization via TypeAdapter with by_alias and exclude_noneThis neatly centralizes serialization for all request models and respects Field aliases (e.g., serialization_alias="from").
tests/unit/test_client.py (2)
6-7: Good: asserting the concrete SendingApi implementationSimple and effective check that the client selects the right SendingApi based on config.
48-48: Nice touch: assigning to '_' to avoid an unused variable warningKeeps the intent clear while triggering the property access (and its validation).
mailtrap/models/mail/__init__.py (2)
1-6: Clean public surface for mail models — LGTMRe-exports are coherent with the new models package. Import paths look consistent with the refactor and tests.
8-15: all is complete and explicit — LGTMThe explicit all keeps the package’s public API tight and predictable.
mailtrap/api/sending.py (1)
20-22: Confirmed:api_datais a@property
Theapi_dataaccessor is defined as a property inmailtrap/models/common.py(line 14), so passingjson=mail.api_datais correct—no parentheses needed.mailtrap/client.py (2)
95-103: Host resolution precedence — LGTMapi_host override, then sandbox, then bulk, then default sending host is a sensible order and aligns with the architecture (Testing API uses GENERAL_HOST; Sending uses SENDING_HOST/SANDBOX_HOST).
104-107: Testing API guardrail — LGTMRequiring account_id when accessing Testing API prevents misconfiguration at runtime.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
tests/unit/api/test_sending.py (3)
93-97: Make request-body assertion robust to JSON formatting differencesRequests may change JSON serialization details (e.g., spacing). Comparing parsed JSON avoids brittleness while still validating payload correctness.
Apply this diff:
- request = responses.calls[0].request # type: ignore - assert request.body == json.dumps(mail.api_data).encode() + request = responses.calls[0].request # type: ignore + assert json.loads(request.body.decode()) == mail.api_data
12-15: Remove unused test constants
ACCOUNT_ID,PROJECT_ID, andINBOX_IDare defined but unused in this module. Trim to reduce noise and potential linter warnings.-ACCOUNT_ID = "321" -PROJECT_ID = 123 -INBOX_ID = "456"
38-52: Add coverage for inbox-scoped sending URLConsider a test verifying that
SendingApi(client, inbox_id="456")posts to/api/send/456. This guards against path regressions for sandbox inbox routing.Here’s a test you can add:
@responses.activate @pytest.mark.parametrize("mail", MAIL_ENTITIES) def test_send_uses_inbox_scoped_path_when_inbox_id_provided(mail: mt.BaseMail) -> None: inbox_id = "456" scoped_url = f"{SEND_FULL_URL}/{inbox_id}" response_body = {"success": True, "message_ids": ["id-1"]} responses.post(scoped_url, json=response_body, status=200) api = SendingApi(client=HttpClient(SENDING_HOST), inbox_id=inbox_id) result = api.send(mail) assert isinstance(result, SendingMailResponse) assert result.success is True assert len(responses.calls) == 1 assert responses.calls[0].request.url == scoped_url # type: ignoremailtrap/api/sending.py (1)
13-18: Nit: rename_api_urlto_api_pathfor clarityThis property returns a path, not a full URL. Renaming improves readability and avoids confusion with
HttpClient._url(...)that builds the full URL.Apply this diff:
- @property - def _api_url(self) -> str: + @property + def _api_path(self) -> str: url = "/api/send" if self._inbox_id: return f"{url}/{self._inbox_id}" return url @@ - def send(self, mail: BaseMail) -> SendingMailResponse: - response = self._client.post(self._api_url, json=mail.api_data) + def send(self, mail: BaseMail) -> SendingMailResponse: + response = self._client.post(self._api_path, json=mail.api_data) return SendingMailResponse(**response)Also applies to: 20-22
mailtrap/client.py (4)
54-58: Consider reusing a single HttpClient/SendingApi instance to benefit from connection poolingConstructing a new HttpClient (requests.Session) on every access can forfeit HTTP connection reuse and add overhead. If you don't rely on mutating
token,sandbox,bulk, orinbox_idafter construction, consider caching the instance.Example change (keeps behavior if you later need to drop caching):
@property def sending_api(self) -> SendingApi: - http_client = HttpClient(host=self._sending_api_host, headers=self.headers) - return SendingApi(client=http_client, inbox_id=self.inbox_id) + try: + return self._sending_api # type: ignore[attr-defined] + except AttributeError: + http_client = HttpClient(host=self._sending_api_host, headers=self.headers) + self._sending_api = SendingApi(client=http_client, inbox_id=self.inbox_id) # type: ignore[attr-defined] + return self._sending_apiNote: If you do need to support token rotation or mode toggling post-init, skip caching or add an explicit refresh method.
19-19: Tighten typing for the public send() result using TypedDict (no runtime change)Using a broad
dict[str, Union[bool, list[str]]]loses key guarantees forsuccessandmessage_ids. A TypedDict keeps the runtime as dict but gives precise typing to users.Apply:
-SEND_ENDPOINT_RESPONSE = dict[str, Union[bool, list[str]]] +class SEND_ENDPOINT_RESPONSE(TypedDict): + success: bool + message_ids: list[str]Additionally add the import:
from typing import TypedDict
68-73: base_url deprecation warning is fine; watch for edge-case api_host valuesThe deprecation path looks good. One caveat: if callers pass
api_hostwith scheme or with a port (e.g.,localhost:8443),base_urlwill producehttps://localhost:8443:443, which is odd even if deprecated.If worth addressing despite deprecation, sanitize
api_hostor document “host only, no scheme/port” in the parameter docstring.
76-85: Avoid double deprecation warnings from api_send_urlCalling
api_send_urltriggers its warning and then callsbase_url, which triggers another warning. Compose the URL directly here to emit a single warning.def api_send_url(self) -> str: warnings.warn( "api_send_url is deprecated and will be removed in a future release.", DeprecationWarning, stacklevel=2, ) - url = f"{self.base_url}/api/send" + # Compose directly to avoid triggering base_url's deprecation warning too + url = f"https://{self._sending_api_host.rstrip('/')}:{self.api_port}/api/send" if self.sandbox and self.inbox_id: return f"{url}/{self.inbox_id}" return url
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
mailtrap/__init__.py(1 hunks)mailtrap/api/sending.py(1 hunks)mailtrap/client.py(3 hunks)mailtrap/models/mail/base.py(1 hunks)tests/unit/api/test_sending.py(1 hunks)tests/unit/test_client.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#31
File: mailtrap/config.py:1-1
Timestamp: 2025-08-12T23:07:25.653Z
Learning: In Mailtrap's API architecture, Testing API resources (Projects, Inboxes, etc.) use the main "mailtrap.io" host, while only email sending functionality uses "sandbox.api.mailtrap.io" as the host.
📚 Learning: 2025-08-12T23:07:25.653Z
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#31
File: mailtrap/config.py:1-1
Timestamp: 2025-08-12T23:07:25.653Z
Learning: In Mailtrap's API architecture, Testing API resources (Projects, Inboxes, etc.) use the main "mailtrap.io" host, while only email sending functionality uses "sandbox.api.mailtrap.io" as the host.
Applied to files:
tests/unit/api/test_sending.pymailtrap/api/sending.py
🧬 Code Graph Analysis (6)
mailtrap/models/mail/base.py (3)
mailtrap/models/common.py (1)
RequestModel(12-18)mailtrap/models/mail/address.py (1)
Address(9-11)mailtrap/models/mail/attachment.py (1)
Attachment(18-27)
tests/unit/test_client.py (1)
mailtrap/client.py (1)
testing_api(46-52)
tests/unit/api/test_sending.py (8)
mailtrap/api/sending.py (2)
SendingApi(8-22)send(20-22)mailtrap/http.py (2)
HttpClient(13-92)post(31-33)mailtrap/models/mail/base.py (2)
SendingMailResponse(24-26)BaseMail(13-20)mailtrap/models/mail/mail.py (1)
mailtrap/models/mail/from_template.py (1)
MailFromTemplate(11-13)mailtrap/exceptions.py (2)
AuthorizationError(18-20)APIError(10-15)mailtrap/client.py (1)
send(59-64)mailtrap/models/common.py (1)
api_data(14-18)
mailtrap/__init__.py (6)
mailtrap/client.py (1)
MailtrapClient(22-121)mailtrap/models/mail/address.py (1)
Address(9-11)mailtrap/models/mail/attachment.py (2)
Attachment(18-27)Disposition(12-14)mailtrap/models/mail/base.py (1)
BaseMail(13-20)mailtrap/models/mail/mail.py (1)
mailtrap/models/mail/from_template.py (1)
MailFromTemplate(11-13)
mailtrap/client.py (5)
mailtrap/api/sending.py (2)
SendingApi(8-22)send(20-22)mailtrap/api/testing.py (1)
TestingApi(7-17)mailtrap/exceptions.py (1)
ClientConfigurationError(5-7)mailtrap/http.py (1)
HttpClient(13-92)mailtrap/models/mail/base.py (2)
BaseMail(13-20)SendingMailResponse(24-26)
mailtrap/api/sending.py (4)
mailtrap/http.py (2)
HttpClient(13-92)post(31-33)mailtrap/models/mail/base.py (2)
BaseMail(13-20)SendingMailResponse(24-26)mailtrap/client.py (1)
send(59-64)mailtrap/models/common.py (1)
api_data(14-18)
🪛 Ruff (0.12.2)
mailtrap/__init__.py
1-1: .client.SEND_ENDPOINT_RESPONSE imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
2-2: .client.MailtrapClient imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
3-3: .exceptions.APIError imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
4-4: .exceptions.AuthorizationError imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
5-5: .exceptions.ClientConfigurationError imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
6-6: .exceptions.MailtrapError imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
7-7: .models.mail.Address imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
8-8: .models.mail.Attachment imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
9-9: .models.mail.BaseMail imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
10-10: .models.mail.Disposition imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
11-11: .models.mail.Mail imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
12-12: .models.mail.MailFromTemplate imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test python3.13 on windows-latest
- GitHub Check: Test python3.12 on windows-latest
- GitHub Check: Test python3.9 on macos-latest
- GitHub Check: Test python3.11 on windows-latest
- GitHub Check: Test python3.10 on windows-latest
- GitHub Check: Test python3.9 on windows-latest
🔇 Additional comments (7)
tests/unit/test_client.py (2)
46-46: Good change: accessing the property triggers validationAssigning
client.testing_apito_ensures the property getter runs and raisesClientConfigurationErroras intended.
46-46: testing_api is correctly declared as a property
Thetesting_apiaccessor inMailtrapClientis preceded by@property(mailtrap/client.py:45), soclient.testing_apiis valid. No changes required.mailtrap/models/mail/base.py (1)
12-26: LGTM: clean, minimal Pydantic dataclasses for request/response
serialization_alias="from"onsenderaligns with API payload.- Optional fields are excluded when None via
api_data, keeping payloads concise.SendingMailResponseshape matches tests and API usage.mailtrap/client.py (4)
45-52: Good: Testing API host selection with account_id guard is correctUsing GENERAL_HOST for Testing API and enforcing
account_idupfront matches the product split (Testing API on main host vs. Sending on dedicated hosts).
59-64: BC preserved: send() still returns a dict-shaped payloadDelegating to SendingApi and serializing via Pydantic while returning a plain dict keeps existing callers unbroken. Good call given the prior discussion.
98-106: Host resolution precedence looks correctPriority: explicit api_host > sandbox host > bulk host > default sending host. Combined with the constructor validation, this is clear and robust.
107-110: Good: Explicit validation for Testing API account_idClear error message and early validation improve DX.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
mailtrap/client.py (5)
19-19: Strengthen response typing with TypedDict instead of a loose dict aliasUsing dict[str, Union[bool, list[str]]] loses key-level guarantees. A TypedDict conveys the exact response shape and improves IDE/helpfulness without breaking runtime.
Apply this diff to introduce a precise type:
-from typing import Optional -from typing import Union -from typing import cast +from typing import Optional, TypedDict, Union, cast @@ -SEND_ENDPOINT_RESPONSE = dict[str, Union[bool, list[str]]] +class SEND_ENDPOINT_RESPONSE(TypedDict): + success: bool + message_ids: list[str]
79-88: api_send_url triggers two DeprecationWarnings from a single accessapi_send_url warns, then calls base_url which warns again. One access produces two warnings; noisy for users.
Inline the URL build to avoid calling base_url:
@property def api_send_url(self) -> str: warnings.warn( "api_send_url is deprecated and will be removed in a future release.", DeprecationWarning, stacklevel=2, ) - url = f"{self.base_url}/api/send" + url = f"https://{self._sending_api_netloc}/api/send" if self.sandbox and self.inbox_id: return f"{url}/{self.inbox_id}" return url
10-13: Single source of truth for host constants to avoid driftYou now import BULK_HOST/SANDBOX_HOST/SENDING_HOST from config, but MailtrapClient still exposes public class attributes with the same intent (Lines 23–27). Keeping duplicate literals risks divergence.
Alias class attributes to the config constants and mark them for removal in the next major:
# add near imports import mailtrap.config as _cfgclass MailtrapClient: - DEFAULT_HOST = "send.api.mailtrap.io" + DEFAULT_HOST = _cfg.SENDING_HOST # DEPRECATED: use mailtrap.config.SENDING_HOST DEFAULT_PORT = 443 - BULK_HOST = "bulk.api.mailtrap.io" - SANDBOX_HOST = "sandbox.api.mailtrap.io" + BULK_HOST = _cfg.BULK_HOST # DEPRECATED: use mailtrap.config.BULK_HOST + SANDBOX_HOST = _cfg.SANDBOX_HOST # DEPRECATED: use mailtrap.config.SANDBOX_HOSTOptionally emit a DeprecationWarning on attribute access in a future PR.
101-109: Name suggests a host, but value is used as netloc_sending_api_host returns just a hostname (no port). Given HttpClient expects a netloc (host[:port]), the name can mislead. Either return a netloc (as suggested) or rename to _sending_api_hostname to avoid ambiguity.
If you adopt _sending_api_netloc, keep _sending_api_host as-is and use the new property wherever a netloc is required.
62-67: Cache the TypeAdapter for SendingMailResponse
SinceSendingMailResponseis defined as a@dataclass(mailtrap/models/mail/base.py:24–26), it doesn’t have a built-inmodel_dump(). Constructing a newTypeAdapteron every call adds unnecessary overhead. You can optimize by creating a single adapter at module scope and reusing it:• At the top of
mailtrap/client.py, add:from typing import cast from pydantic import TypeAdapter from mailtrap.models.mail.base import SendingMailResponse, SEND_ENDPOINT_RESPONSE _SENDING_MAIL_RESPONSE_ADAPTER = TypeAdapter(SendingMailResponse)• Then update the
sendmethod (lines 62–67) to:- def send(self, mail: BaseMail) -> SEND_ENDPOINT_RESPONSE: - sending_response = self.sending_api.send(mail) - return cast( - SEND_ENDPOINT_RESPONSE, - TypeAdapter(SendingMailResponse).dump_python(sending_response), - ) + def send(self, mail: BaseMail) -> SEND_ENDPOINT_RESPONSE: + sending_response = self.sending_api.send(mail) + return cast( + SEND_ENDPOINT_RESPONSE, + _SENDING_MAIL_RESPONSE_ADAPTER.dump_python(sending_response), + )This change reduces per-call instantiation cost while preserving behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
mailtrap/client.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#31
File: mailtrap/config.py:1-1
Timestamp: 2025-08-12T23:07:25.653Z
Learning: In Mailtrap's API architecture, Testing API resources (Projects, Inboxes, etc.) use the main "mailtrap.io" host, while only email sending functionality uses "sandbox.api.mailtrap.io" as the host.
🧬 Code graph analysis (1)
mailtrap/client.py (4)
mailtrap/api/sending.py (2)
SendingApi(8-22)send(20-22)mailtrap/exceptions.py (1)
ClientConfigurationError(5-7)mailtrap/http.py (1)
HttpClient(13-92)mailtrap/models/mail/base.py (2)
BaseMail(13-20)SendingMailResponse(24-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test python3.10 on windows-latest
- GitHub Check: Test python3.13 on windows-latest
- GitHub Check: Test python3.12 on windows-latest
- GitHub Check: Test python3.9 on windows-latest
- GitHub Check: Test python3.11 on windows-latest
- GitHub Check: Test python3.9 on macos-latest
🔇 Additional comments (2)
mailtrap/client.py (2)
71-76: Deprecation policy: good use of DeprecationWarning and stacklevelThe deprecation warning reads clearly and uses the correct category and stacklevel. No action needed.
110-112: Validates Testing API preconditions up frontRequiring account_id before exposing testing_api matches the documented split between Testing API (mailtrap.io) and Sending API (sandbox/bulk). Good guardrail.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mailtrap/client.py (1)
23-27: Public host aliases retained for backward compatibility — LGTMKeeping DEFAULT_HOST, BULK_HOST, and SANDBOX_HOST as public class attributes mapped to config constants aligns with the prior decision to avoid breaking importers.
🧹 Nitpick comments (2)
mailtrap/client.py (2)
57-61: Avoid re-creating HttpClient/session on every access; cache SendingApiEach call builds a new HttpClient (new requests.Session), defeating connection pooling. Cache the instance lazily inside the property to keep the same session across sends.
Apply this diff:
@property def sending_api(self) -> SendingApi: - http_client = HttpClient(host=self._sending_api_host, headers=self.headers) - return SendingApi(client=http_client, inbox_id=self.inbox_id) + api = getattr(self, "_sending_api_cached", None) + if api is None: + http_client = HttpClient(host=self._sending_api_host, headers=self.headers) + api = SendingApi(client=http_client, inbox_id=self.inbox_id) + setattr(self, "_sending_api_cached", api) + return apiNote: If you anticipate token rotation per client instance (unlikely), expose an explicit invalidate/close to rebuild the cached client.
1-4: Strengthen typing of SEND_ENDPOINT_RESPONSE with TypedDict (no runtime change)Preserves the public “dict” shape while giving precise static types to callers and tests.
Apply these diffs:
-from typing import Optional -from typing import Union -from typing import cast +from typing import Optional +from typing import Union +from typing import cast +from typing import TypedDict-SEND_ENDPOINT_RESPONSE = dict[str, Union[bool, list[str]]] +class SendEndpointResponse(TypedDict): + success: bool + message_ids: list[str] + +SEND_ENDPOINT_RESPONSE = SendEndpointResponseOptional (only if SendingMailResponse is a Pydantic BaseModel): you can simplify dumping in send() to
sending_response.model_dump()and drop TypeAdapter for readability without changing output.Also applies to: 19-19
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
mailtrap/client.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#33
File: mailtrap/client.py:57-61
Timestamp: 2025-08-21T09:11:51.722Z
Learning: The api_port parameter in MailtrapClient is deprecated and will be removed soon. It's being kept temporarily for backward compatibility but is intentionally not being used in the new sending_api implementation.
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#31
File: mailtrap/config.py:1-1
Timestamp: 2025-08-12T23:07:25.653Z
Learning: In Mailtrap's API architecture, Testing API resources (Projects, Inboxes, etc.) use the main "mailtrap.io" host, while only email sending functionality uses "sandbox.api.mailtrap.io" as the host.
📚 Learning: 2025-08-21T09:11:51.722Z
Learnt from: Ihor-Bilous
PR: railsware/mailtrap-python#33
File: mailtrap/client.py:57-61
Timestamp: 2025-08-21T09:11:51.722Z
Learning: The api_port parameter in MailtrapClient is deprecated and will be removed soon. It's being kept temporarily for backward compatibility but is intentionally not being used in the new sending_api implementation.
Applied to files:
mailtrap/client.py
🧬 Code graph analysis (1)
mailtrap/client.py (4)
mailtrap/api/sending.py (2)
SendingApi(8-22)send(20-22)mailtrap/exceptions.py (1)
ClientConfigurationError(5-7)mailtrap/http.py (1)
HttpClient(13-92)mailtrap/models/mail/base.py (2)
BaseMail(13-20)SendingMailResponse(24-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (python)
- GitHub Check: Test python3.13 on windows-latest
- GitHub Check: Test python3.10 on macos-latest
- GitHub Check: Test python3.12 on windows-latest
- GitHub Check: Test python3.9 on macos-latest
- GitHub Check: Test python3.11 on ubuntu-latest
- GitHub Check: Test python3.10 on windows-latest
- GitHub Check: Test python3.11 on windows-latest
- GitHub Check: Test python3.9 on windows-latest
🔇 Additional comments (4)
mailtrap/client.py (4)
62-67: Send flow and backward-compatible dict response — LGTMDelegating to SendingApi and serializing to the existing dict contract is clean and keeps the external API stable.
101-109: Host resolution logic is correct and aligns with config separationSelecting SANDBOX_HOST for sandbox, BULK_HOST for bulk, otherwise SENDING_HOST, while allowing api_host override, matches the intended architecture.
110-112: Explicit guard for Testing API account_id — goodClear configuration error improves DX when using Testing API.
80-88: [runnable_scripts]#!/bin/bash # Check parametrization for api_send_url tests to ensure sandbox/inbox_id case is covered. rg -nC2 "@pytest.mark.parametrize" tests/unit/test_client.py
Motivation
In this PR I added Sandbox sending email feature and refactored default sending functionality using new design structure.
Changes
How to test
See examples/sending..py
Images and GIFs
Summary by CodeRabbit
New Features
Refactor
Tests