Fix issue #29. Add support of Emails Sandbox (Testing) API: Projects #31
Conversation
WalkthroughThis update introduces a new API client architecture for Mailtrap, including foundational HTTP client logic, configuration, and Pydantic-based data models for projects, inboxes, and permissions. It adds a modular resource API for project CRUD operations, comprehensive unit tests, and usage examples. Supporting documentation and requirements are updated accordingly. Changes
Sequence Diagram(s)MailtrapApiClient Project CRUD FlowsequenceDiagram
participant User
participant MailtrapApiClient
participant HttpClient
participant TestingApi
participant ProjectsApi
participant Mailtrap API
User->>MailtrapApiClient: Initialize(token)
User->>MailtrapApiClient: testing_api(account_id)
MailtrapApiClient->>HttpClient: Initialize(host, headers)
MailtrapApiClient->>TestingApi: Initialize(HttpClient, account_id)
User->>TestingApi: projects
TestingApi->>ProjectsApi: Initialize(HttpClient, account_id)
User->>ProjectsApi: get_list()/create()/update()/delete()
ProjectsApi->>HttpClient: get/post/patch/delete (API call)
HttpClient->>Mailtrap API: HTTP request
Mailtrap API-->>HttpClient: HTTP response
HttpClient-->>ProjectsApi: Parsed response or error
ProjectsApi-->>User: Project(s)/DeletedObject/Error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧬 Code Graph Analysis (1)mailtrap/http.py (2)
🔇 Additional comments (10)
✨ 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 comments)
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: 5
🧹 Nitpick comments (4)
mailtrap/models/common.py (1)
1-3: Consider enhancing the class for better maintainability.The implementation is functional but could benefit from some optional improvements:
+from dataclasses import dataclass + +@dataclass -class DeletedObject: - def __init__(self, id: str): - self.id = id +class DeletedObject: + """Represents a deleted object with its identifier.""" + id: strAlternatively, if you prefer to keep the current approach, consider adding a
__repr__method for better debugging:class DeletedObject: + """Represents a deleted object with its identifier.""" + def __init__(self, id: str): self.id = id + + def __repr__(self) -> str: + return f"DeletedObject(id='{self.id}')"tests/unit/api/test_projects.py (1)
100-116: Consider validating the request payload.The test correctly validates the response, but could also verify that the correct request payload is sent.
@responses.activate def test_create_should_return_new_project( self, client: ProjectsApiClient, sample_project_dict: Dict ) -> None: responses.add( responses.POST, BASE_URL, json=sample_project_dict, status=201, ) project = client.create(ACCOUNT_ID, name="New Project") assert isinstance(project, Project) assert project.name == "Test Project" + # Verify the request payload + assert len(responses.calls) == 1 + request_json = responses.calls[0].request.json() + assert request_json == {"project": {"name": "New Project"}}mailtrap/models/projects.py (1)
1-50: Consider adding docstrings and repr methods for better maintainability.While the implementation is solid, consider these optional improvements:
class ShareLinks: + """Links for sharing project access.""" + def __init__(self, admin: str, viewer: str): self.admin = admin self.viewer = viewer + + def __repr__(self) -> str: + return f"ShareLinks(admin='{self.admin}', viewer='{self.viewer}')" class Permissions: + """Permission flags for project access control.""" + def __init__( self, can_read: bool, can_update: bool, can_destroy: bool, can_leave: bool ): self.can_read = can_read self.can_update = can_update self.can_destroy = can_destroy self.can_leave = can_leave + + def __repr__(self) -> str: + return (f"Permissions(can_read={self.can_read}, can_update={self.can_update}, " + f"can_destroy={self.can_destroy}, can_leave={self.can_leave})") class Project: + """Represents a Mailtrap project with its metadata and permissions.""" + def __init__( self, id: str, name: str, share_links: ShareLinks, inboxes: List[Dict[str, Any]], permissions: Permissions ): self.id = id self.name = name self.share_links = share_links self.inboxes = inboxes self.permissions = permissions + + def __repr__(self) -> str: + return f"Project(id='{self.id}', name='{self.name}')"mailtrap/api/base.py (1)
44-46: Consider adding abstract methods to justify ABC inheritance.The class inherits from ABC but has no abstract methods, which static analysis correctly flags. Consider whether this class should define abstract methods that subclasses must implement, or if ABC inheritance is necessary.
If you want to keep the ABC inheritance to enforce the pattern, consider adding an abstract method like:
from abc import ABC, abstractmethod class BaseHttpApiClient(ABC): def __init__(self, session: Session): self.session = session @abstractmethod def _build_url(self, *args: Any) -> str: """Build API endpoint URL. Must be implemented by subclasses.""" passAlternatively, if no abstract interface is needed, simply remove the ABC inheritance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
mailtrap/api/base.py(1 hunks)mailtrap/api/projects.py(1 hunks)mailtrap/client.py(2 hunks)mailtrap/constants.py(1 hunks)mailtrap/models/common.py(1 hunks)mailtrap/models/projects.py(1 hunks)tests/unit/api/test_projects.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
mailtrap/client.py (3)
mailtrap/api/projects.py (2)
ProjectsApiClient(9-50)update(37-43)mailtrap/mail/base.py (2)
BaseMail(11-53)api_data(33-44)mailtrap/mail/base_entity.py (1)
api_data(9-10)
tests/unit/api/test_projects.py (4)
mailtrap/api/projects.py (6)
ProjectsApiClient(9-50)get_list(15-20)get_by_id(22-27)create(29-35)update(37-43)delete(45-50)mailtrap/exceptions.py (1)
APIError(10-15)mailtrap/models/common.py (1)
DeletedObject(1-3)mailtrap/models/projects.py (1)
Project(24-50)
mailtrap/api/base.py (2)
mailtrap/exceptions.py (2)
APIError(10-15)AuthorizationError(18-20)mailtrap/client.py (1)
_handle_failed_response(87-94)
mailtrap/api/projects.py (3)
mailtrap/api/base.py (3)
BaseHttpApiClient(44-70)HttpMethod(12-17)_request(48-54)mailtrap/models/common.py (1)
DeletedObject(1-3)mailtrap/models/projects.py (2)
Project(24-50)from_dict(40-50)
🪛 Ruff (0.12.2)
mailtrap/api/base.py
44-44: BaseHttpApiClient is an abstract base class, but it has no abstract methods or properties
(B024)
63-63: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (23)
mailtrap/constants.py (1)
1-1: LGTM! Clean constant definition.The constant is well-named and provides a centralized location for the Mailtrap host configuration.
mailtrap/client.py (4)
7-7: LGTM! Clean integration of the Projects API.The import follows the existing pattern and properly integrates the new functionality.
38-39: Excellent improvement with persistent HTTP session.Creating a persistent
requests.Sessionand pre-configuring headers is a great optimization that will:
- Enable connection pooling and reuse
- Reduce overhead from repeated header configuration
- Improve overall performance for multiple API calls
41-43: Clean property implementation for Projects API access.The property provides a clean interface to access project-related functionality while sharing the same HTTP session for consistency.
46-46: Good use of the persistent session.Using the session's
postmethod instead of the standalonerequests.postleverages the connection pooling and pre-configured headers effectively.tests/unit/api/test_projects.py (4)
17-19: LGTM! Good fixture setup.Clean fixture providing a client instance for testing.
22-38: Excellent comprehensive test data fixture.The sample project dictionary covers all the required fields and provides realistic test data that matches the Project model structure.
41-58: Thorough test for get_list functionality.Good coverage testing both the list structure and individual Project instances.
61-77: Good error handling test.Properly tests the 404 error scenario and validates exception handling.
mailtrap/models/projects.py (5)
1-1: LGTM! Correct Python 3.6 compatible type annotations.Good use of
List[Dict[str, Any]]instead oflist[dict[str, Any]]for Python 3.6 compatibility.
4-7: LGTM! Clean data model.Simple and focused class for share links data.
10-21: Well-structured permissions model.Good separation of permission flags with clear naming.
24-37: Solid project model structure.Clean data model with appropriate type hints and clear attribute organization.
39-50: Excellent deserialization method.The
from_dictmethod properly handles nested object creation and provides sensible defaults.mailtrap/api/projects.py (6)
1-6: LGTM! Clean imports and proper typing.The imports are well-organized and include all necessary components. The use of
castfrom typing module is appropriate for the type casting needs in this implementation.
9-13: LGTM! Well-designed URL building method.The
_build_urlhelper method properly constructs API URLs with account scoping. The use of*partsparameter allows for flexible URL construction for different endpoints.
15-20: LGTM! Proper implementation of list retrieval.The method correctly uses type casting and list comprehension to convert the API response into
Projectobjects. The explicit casting helps with type safety.
22-27: LGTM! Clean single project retrieval.The method follows the same pattern as
get_listwith proper type casting and model conversion.
29-35: LGTM! Proper project creation with nested JSON structure.The method correctly structures the request payload with the nested
{"project": {"name": name}}format as expected by the API.
37-43: LGTM! Consistent update implementation.The update method follows the same pattern as create, using PATCH method appropriately for partial updates.
mailtrap/api/base.py (3)
1-9: LGTM! Proper imports and type definitions.The imports include all necessary components and the type aliases provide clear documentation for expected response formats.
20-41: LGTM! Robust error extraction logic.The
_extract_errorsfunction handles multiple error response formats gracefully:
- List of errors
- Dictionary of field-specific errors
- Single error values
- Fallback for unknown formats
This provides good flexibility for different API endpoint error structures.
48-54: LGTM! Well-designed request method with proper type handling.The
_requestmethod provides a clean abstraction for HTTP requests with proper type casting and error handling delegation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
mailtrap/api/base.py (1)
60-63: Fix exception chaining for better error diagnostics.The
ValueErrorcaught during JSON parsing should be chained to preserve the original error context for debugging.Apply this fix:
try: data = response.json() - except ValueError: - raise APIError(status_code, errors=["Unknown Error"]) + except ValueError as e: + raise APIError(status_code, errors=["Unknown Error"]) from e
🧹 Nitpick comments (1)
mailtrap/api/base.py (1)
44-46: Consider the abstract base class design.The class inherits from
ABCbut doesn't define any abstract methods. If this is intended to be a true abstract base class, consider adding abstract methods that subclasses must implement. Otherwise, regular inheritance withoutABCmight be more appropriate.If you want to enforce an interface, consider adding abstract methods:
class BaseHttpApiClient(ABC): def __init__(self, session: Session): self.session = session + + @abstractmethod + def get_base_url(self) -> str: + """Return the base URL for this API client.""" + passOr remove ABC inheritance if not needed:
-class BaseHttpApiClient(ABC): +class BaseHttpApiClient: def __init__(self, session: Session): self.session = session
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mailtrap/api/base.py(1 hunks)mailtrap/api/projects.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mailtrap/api/projects.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
mailtrap/api/base.py (2)
mailtrap/exceptions.py (2)
APIError(10-15)AuthorizationError(18-20)mailtrap/client.py (1)
_handle_failed_response(87-94)
🪛 Ruff (0.12.2)
mailtrap/api/base.py
44-44: BaseHttpApiClient is an abstract base class, but it has no abstract methods or properties
(B024)
63-63: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (4)
mailtrap/api/base.py (4)
1-10: LGTM!The imports are well-organized and the type aliases provide clear documentation for the expected response formats.
12-17: LGTM!The HttpMethod enum is correctly implemented with all standard HTTP methods properly defined.
20-41: Excellent error handling flexibility.This function effectively handles various error response formats that different API endpoints might return, with appropriate fallbacks for edge cases. The flattening of nested error dictionaries provides clear, actionable error messages.
48-54: Solid request method design.The method provides a clean interface for making HTTP requests with proper error handling delegation. The use of
cast()for type checking is reasonable given the shared method design, though future refinements to typing could be considered as mentioned in the PR description.
…t for Projects API
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
mailtrap/models/projects.py (1)
1-20: Type annotation consistency issue already identifiedA previous review has already identified the need to unify type annotations for Python 3.6 compatibility. The use of
list[Inbox]on line 19 should be changed toList[Inbox]with appropriate imports.tests/unit/api/test_projects.py (1)
159-172: Verify the HTTP method enum typo is fixedA previous review identified that the implementation uses
HttpMethod.DELTEinstead ofHttpMethod.DELETE. This test expects the correct DELETE method to work.
🧹 Nitpick comments (5)
examples/testing/projects.py (2)
1-3: Avoid sys.path manipulation in examplesModifying
sys.pathdirectly is not a recommended practice. Consider using relative imports or running the example as a module.Instead of modifying sys.path, run the example using:
python -m examples.testing.projectsOr use a more robust path resolution:
from pathlib import Path import sys sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent))
20-30: Add error handling to the exampleThe example doesn't demonstrate error handling, which would be helpful for users to understand how to handle API failures.
Consider wrapping API calls in try-except blocks:
try: created_project = api.create(project_name=project_name) print(f"Created project: {created_project.name}") projects = api.get_list() print(f"Found {len(projects)} projects") # ... rest of the operations except APIError as e: print(f"API error occurred: {e}") except Exception as e: print(f"Unexpected error: {e}")tests/unit/api/test_projects.py (1)
45-45: Fix method signature for consistencyThe parameter type annotation is missing the import.
-def test_get_list_should_return_project_list( - self, client: ProjectsApi, sample_project_dict: dict -) -> None: +def test_get_list_should_return_project_list( + self, client: ProjectsApi, sample_project_dict: Dict[str, Any] +) -> None:Apply similar changes to other test methods that use the
sample_project_dictparameter.mailtrap/http.py (2)
50-52: Enhance error message with actual response typeThe error message could be more helpful by including what type was actually received.
if not isinstance(data, expected_type): - raise APIError(response.status_code, errors=[f"Expected response type {expected_type.__name__}"]) + raise APIError( + response.status_code, + errors=[f"Expected response type {expected_type.__name__}, got {type(data).__name__}"] + )
105-120: Consider extracting nested function for better testabilityThe
flatten_errorsfunction could be extracted as a module-level function to improve testability and reusability.-def _extract_errors(data: dict[str, Any]) -> list[str]: - def flatten_errors(errors: Any) -> list[str]: - if isinstance(errors, list): - return [str(error) for error in errors] - - if isinstance(errors, dict): - flat_errors = [] - for key, value in errors.items(): - if isinstance(value, list): - flat_errors.extend([f"{key}: {v}" for v in value]) - else: - flat_errors.append(f"{key}: {value}") - return flat_errors - - return [str(errors)] +def _flatten_errors(errors: Any) -> list[str]: + if isinstance(errors, list): + return [str(error) for error in errors] + + if isinstance(errors, dict): + flat_errors = [] + for key, value in errors.items(): + if isinstance(value, list): + flat_errors.extend([f"{key}: {v}" for v in value]) + else: + flat_errors.append(f"{key}: {value}") + return flat_errors + + return [str(errors)] + + +def _extract_errors(data: dict[str, Any]) -> list[str]:Then update the calls within the function:
if "errors" in data: - return flatten_errors(data["errors"]) + return _flatten_errors(data["errors"]) if "error" in data: - return flatten_errors(data["error"]) + return _flatten_errors(data["error"])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
examples/testing/projects.py(1 hunks)mailtrap/api/resources/projects.py(1 hunks)mailtrap/api/testing.py(1 hunks)mailtrap/client.py(2 hunks)mailtrap/config.py(1 hunks)mailtrap/http.py(1 hunks)mailtrap/models/base.py(1 hunks)mailtrap/models/inboxes.py(1 hunks)mailtrap/models/permissions.py(1 hunks)mailtrap/models/projects.py(1 hunks)tests/unit/api/test_projects.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- mailtrap/config.py
- mailtrap/models/permissions.py
- mailtrap/api/testing.py
- mailtrap/models/inboxes.py
🚧 Files skipped from review as they are similar to previous changes (1)
- mailtrap/client.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
mailtrap/api/resources/projects.py (5)
mailtrap/http.py (5)
HttpClient(13-102)get(60-66)post(76-82)patch(92-98)delete(100-102)mailtrap/models/base.py (1)
DeletedObject(41-42)mailtrap/api/testing.py (1)
projects(12-13)mailtrap/models/projects.py (1)
Project(15-20)tests/unit/api/test_projects.py (1)
client(19-20)
mailtrap/models/projects.py (3)
mailtrap/models/base.py (1)
BaseModel(9-36)mailtrap/models/inboxes.py (1)
Inbox(8-31)mailtrap/models/permissions.py (1)
Permissions(7-11)
mailtrap/http.py (3)
mailtrap/exceptions.py (2)
APIError(10-15)AuthorizationError(18-20)mailtrap/client.py (2)
headers(74-81)_host(84-91)mailtrap/api/resources/projects.py (2)
update(26-31)delete(33-37)
🪛 Ruff (0.12.2)
mailtrap/http.py
33-33: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
mailtrap/api/resources/projects.py
Outdated
| def create(self, project_name: str) -> Project: | ||
| response = self.client.post( | ||
| f"/api/accounts/{self.account_id}/projects", | ||
| json={"project": {"name": project_name}}, | ||
| ) | ||
| return Project.from_dict(response) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for project_name
The create and update methods should validate that project_name is not empty or None before making the API call.
def create(self, project_name: str) -> Project:
if not project_name or not project_name.strip():
raise ValueError("Project name cannot be empty")
response = self.client.post(
f"/api/accounts/{self.account_id}/projects",
json={"project": {"name": project_name}},
)
return Project.from_dict(response)
def update(self, project_id: str, project_name: str) -> Project:
if not project_name or not project_name.strip():
raise ValueError("Project name cannot be empty")
response = self.client.patch(
f"/api/accounts/{self.account_id}/projects/{project_id}",
json={"project": {"name": project_name}},
)
return Project.from_dict(response)Also applies to: 26-31
🤖 Prompt for AI Agents
In mailtrap/api/resources/projects.py around lines 19 to 24 and 26 to 31, add
input validation in the create and update methods to check if project_name is
empty or None before making the API call. Raise a ValueError with a clear
message if project_name is invalid. This prevents unnecessary API calls with
invalid data.
mailtrap/http.py
Outdated
| from typing import Any, Optional, Type, TypeVar | ||
| from typing import NoReturn |
There was a problem hiding this comment.
Type annotations incompatible with Python 3.6
The code uses lowercase generic type annotations (e.g., dict[str, Any], list[dict[str, Any]]) which require Python 3.9+. According to the PR objectives, the SDK should support Python 3.6, which requires importing and using Dict and List from the typing module.
Apply this diff to fix Python 3.6 compatibility:
-from typing import Any, Optional, Type, TypeVar
-from typing import NoReturn
+from typing import Any, Dict, List, NoReturn, Optional, Type, TypeVarThen update all occurrences throughout the file:
dict[str, Any]→Dict[str, Any]list[str]→List[str]list[dict[str, Any]]→List[Dict[str, Any]]
Also applies to: 17-17, 54-54, 57-57, 63-63, 72-72, 79-79, 87-87, 95-95, 100-100, 105-105
🤖 Prompt for AI Agents
In mailtrap/http.py at lines 1-2 and also lines 17, 54, 57, 63, 72, 79, 87, 95,
100, and 105, the code uses lowercase generic type annotations like dict[str,
Any] and list[dict[str, Any]], which are incompatible with Python 3.6. To fix
this, import Dict and List from the typing module instead of using lowercase
generics, then replace all occurrences of dict[str, Any] with Dict[str, Any],
list[str] with List[str], and list[dict[str, Any]] with List[Dict[str, Any]]
throughout the file to ensure Python 3.6 compatibility.
mailtrap/models/base.py
Outdated
| @dataclass | ||
| class BaseModel: | ||
| @classmethod | ||
| def from_dict(cls: Type[T], data: dict[str, Any]) -> T: |
There was a problem hiding this comment.
Type annotation incompatible with Python 3.6
The method signature uses dict[str, Any] which requires Python 3.9+. For Python 3.6 compatibility, use Dict[str, Any] from the typing module.
Apply this diff to fix the compatibility issue:
-from typing import Any, Type, TypeVar, get_args, get_origin
+from typing import Any, Dict, Type, TypeVar, get_args, get_origin- def from_dict(cls: Type[T], data: dict[str, Any]) -> T:
+ def from_dict(cls: Type[T], data: Dict[str, Any]) -> T:- values: dict[str, Any] = {}
+ values: Dict[str, Any] = {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def from_dict(cls: Type[T], data: dict[str, Any]) -> T: | |
| from typing import Any, Dict, Type, TypeVar, get_args, get_origin | |
| @classmethod | |
| def from_dict(cls: Type[T], data: Dict[str, Any]) -> T: | |
| values: Dict[str, Any] = {} | |
| # …rest of implementation… |
🤖 Prompt for AI Agents
In mailtrap/models/base.py at line 11, the type annotation dict[str, Any] is
incompatible with Python 3.6. Replace dict[str, Any] with Dict[str, Any] and
import Dict from the typing module to ensure compatibility with Python 3.6.
… validation for create and update actions
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/unit/api/test_projects.py (1)
1-1: Type annotations incompatible with Python 3.6The code uses
dict[str, Any]andlist[str]which require Python 3.9+. For Python 3.6 compatibility, import and useDictandListfrom typing.-from typing import Any +from typing import Any, Dict, List-def sample_project_dict() -> dict[str, Any]: +def sample_project_dict() -> Dict[str, Any]:- def test_create_should_raise_validation_error_on_pydantic_validation( - self, client: ProjectsApi, project_name: str, expected_errors: list[str] - ) -> None: + def test_create_should_raise_validation_error_on_pydantic_validation( + self, client: ProjectsApi, project_name: str, expected_errors: List[str] + ) -> None:Also applies to line 238 where
list[str]is used again.Also applies to: 25-25, 172-172
mailtrap/http.py (1)
1-4: Type annotations incompatible with Python 3.6The code uses lowercase generic type annotations which require Python 3.9+. According to the PR objectives, the SDK should support Python 3.6.
Also applies to: 20-20
🧹 Nitpick comments (1)
mailtrap/client.py (1)
22-43: Consider thread safety and multi-tenant implications of class-level token.The class-level
_default_tokenis shared across all instances and threads. This design has implications:
- Not thread-safe if tokens need to be changed at runtime
- Prevents using different tokens for different client instances in the same process
- Could cause issues in multi-tenant applications
Consider either:
- Adding thread-safe token management (e.g., using threading.local())
- Supporting both class-level default and instance-level override tokens
- Documenting the single-token limitation clearly
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.gitignore(1 hunks)README.md(2 hunks)examples/testing/projects.py(1 hunks)mailtrap/api/resources/projects.py(1 hunks)mailtrap/api/testing.py(1 hunks)mailtrap/client.py(4 hunks)mailtrap/http.py(1 hunks)mailtrap/schemas/base.py(1 hunks)mailtrap/schemas/inboxes.py(1 hunks)mailtrap/schemas/permissions.py(1 hunks)mailtrap/schemas/projects.py(1 hunks)requirements.test.txt(1 hunks)requirements.txt(1 hunks)tests/unit/api/test_projects.py(1 hunks)tests/unit/test_client.py(3 hunks)
✅ Files skipped from review due to trivial changes (6)
- .gitignore
- README.md
- mailtrap/schemas/base.py
- requirements.txt
- requirements.test.txt
- mailtrap/schemas/inboxes.py
🚧 Files skipped from review as they are similar to previous changes (3)
- mailtrap/api/testing.py
- mailtrap/api/resources/projects.py
- examples/testing/projects.py
🔇 Additional comments (6)
mailtrap/schemas/permissions.py (1)
1-8: LGTM!Clean and straightforward Pydantic model for permissions. The boolean fields appropriately represent the permission flags.
tests/unit/test_client.py (2)
29-32: Test helper correctly updated for class-level token management.The modification to use
configure_access_tokenclass method instead of passing token as instance parameter aligns with the refactored client design.
87-93: Header access correctly updated to use class method.The tests now properly use
get_default_headers()class method instead of accessing instanceheadersattribute, consistent with the new token management approach.Also applies to: 107-108
mailtrap/schemas/projects.py (1)
1-23: Well-structured Pydantic models with appropriate validation.The schema definitions are clean and follow best practices:
- Proper use of nested models for complex data structures
- Appropriate field validation constraints on
ProjectInput.name- Clear separation between input validation (
ProjectInput) and data representation (Project)tests/unit/api/test_projects.py (1)
43-312: Comprehensive test coverage with excellent structure.The test implementation demonstrates best practices:
- Thorough coverage of all CRUD operations
- Proper error handling tests for various HTTP status codes
- Good use of parametrized tests to reduce code duplication
- Validation error testing for edge cases
- Clean fixture usage
mailtrap/client.py (1)
44-50: Clean implementation of testing API factory method.The
get_testing_apimethod provides a convenient way to create TestingApi instances with proper configuration and shared HTTP client.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
mailtrap/__init__.py (1)
1-14: Expose public symbols via__all__to silence unused-import warningsImports at module level are meant for re-export, but Ruff still flags them unless they’re listed in
__all__. Add/extend the tuple so static analysis tools understand the intent.-from .client import MailtrapApiClient -from .client import MailtrapClient +from .client import MailtrapApiClient, MailtrapClient + +# Public interface +__all__ = ( + "MailtrapApiClient", + "MailtrapClient", + "APIError", + "AuthorizationError", + "ClientConfigurationError", + "MailtrapError", + "Address", + "Attachment", + "BaseEntity", + "BaseMail", + "Disposition", + "Mail", + "MailFromTemplate", +)
🧹 Nitpick comments (1)
mailtrap/models/projects.py (1)
13-18: Consider validating share-link URLs
ShareLinks.adminand.viewercontain URLs. Declaring them asHttpUrl(orAnyUrl) gives automatic validation and clearer intent.-class ShareLinks(BaseModel): - admin: str - viewer: str +from pydantic import HttpUrl + + +class ShareLinks(BaseModel): + admin: HttpUrl + viewer: HttpUrl
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
examples/testing/projects.py(1 hunks)mailtrap/__init__.py(1 hunks)mailtrap/api/resources/projects.py(1 hunks)mailtrap/client.py(2 hunks)mailtrap/config.py(1 hunks)mailtrap/http.py(1 hunks)mailtrap/models/base.py(1 hunks)mailtrap/models/inboxes.py(1 hunks)mailtrap/models/permissions.py(1 hunks)mailtrap/models/projects.py(1 hunks)pyproject.toml(1 hunks)tests/unit/api/test_projects.py(1 hunks)tests/unit/test_client.py(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- mailtrap/models/base.py
- mailtrap/config.py
- mailtrap/models/permissions.py
- pyproject.toml
- mailtrap/models/inboxes.py
🚧 Files skipped from review as they are similar to previous changes (6)
- mailtrap/client.py
- examples/testing/projects.py
- tests/unit/test_client.py
- mailtrap/api/resources/projects.py
- mailtrap/http.py
- tests/unit/api/test_projects.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
mailtrap/models/projects.py (2)
mailtrap/models/inboxes.py (1)
Inbox(8-33)mailtrap/models/permissions.py (1)
Permissions(4-8)
🪛 Ruff (0.12.2)
mailtrap/__init__.py
1-1: .client.MailtrapApiClient imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
mailtrap/models/projects.py
2-2: pydantic.Field imported but unused
Remove unused import: pydantic.Field
(F401)
|
|
||
| def delete(self, path: str) -> Any: | ||
| response = self._session.delete(self._url(path), timeout=self._timeout) | ||
| return self._process_response(response) |
There was a problem hiding this comment.
Let's move all public methods above private ones
Motivation
The first PR with base structure and api for projects.
Changes
How to test
In the terminal, run the following command from the root of the project:
tox. This command is important because the Mailtrap SDK will be built and installed locally. Then, it will be possible to use Mailtrap in the examples.In the file examples/testing/projects.py, change the values of the variables API_TOKEN and ACCOUNT_ID, then run the command
python examples/testing/projects.py.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
.vscode/to.gitignore.pydanticand improved test requirements.