Conversation
- Add 13 v2 Pydantic models (V2Address, V2Stop, V2Item, V2Price, etc.) - Add BrengerV2APIClient with endpoints: quote, create_shipment, get_shipment_status, cancel_shipment, get_refund - Use exclude_none=True in JSON serialization to avoid sending null for optional fields - V2Item.category defaults to "other" (required by API) - Bump version to 2.0.0 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds support for the Brenger partner API v2 by introducing new Pydantic models and a dedicated client class. The implementation maintains backward compatibility by keeping all v1 code intact while adding parallel v2 functionality.
Changes:
- Added v2 Pydantic models (V2Address, V2Stop, V2Item, V2Amount, V2Price, V2QuoteRequest, V2ShipmentCreateRequest, and various response models)
- Added BrengerV2APIClient with methods for quote, create_shipment, get_shipment_status, cancel_shipment, and get_refund
- Bumped version from 0.1.8 to 2.0.0
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 18 comments.
| File | Description |
|---|---|
| pyproject.toml | Version bumped to 2.0.0 |
| brenger/models.py | Added 14 new V2 model classes for the v2 API including request/response models for quotes, shipments, status, refunds, and webhooks |
| brenger/client.py | Added BrengerV2APIClient class with 5 API endpoint methods and error handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
brenger/client.py
Outdated
| response_json = response.json() | ||
| error_description = response_json.get("description", "An error occurred") | ||
| error_hint = response_json.get("hint", "Hint not provided") | ||
| error_validation = response_json.get( | ||
| "validation_errors", "Validation not provided" | ||
| ) |
There was a problem hiding this comment.
The error handling assumes response.json() will always succeed on error responses (line 123). If the server returns a non-JSON response for a 4xx error, this will raise a JSONDecodeError and mask the actual error. Consider adding a try-except block around response.json() to handle cases where the error response is not valid JSON, falling back to response.text or a generic error message.
| self._handle_response_errors(response) | ||
| logger.info("Quote retrieved successfully") | ||
| return V2QuoteResponse(**response.json()) |
There was a problem hiding this comment.
Similar to create_shipment, response.json() is called twice here - once for logging (line 82) and once for returning the parsed response. While requests.Response.json() typically caches the result, it's more efficient and clearer to store the parsed JSON in a variable and reuse it.
| f" Status code: {response.status_code} - " | ||
| f"Error description: {error_description} -" | ||
| f" Error hint: {error_hint} - " | ||
| f" validation errors: {error_validation}" |
There was a problem hiding this comment.
The error message formatting has inconsistent spacing. Line 130 has an extra space at the beginning (" Status code:"), line 131 has no space after the hyphen ("description: {error_description} -"), and line 132 has spaces around the hyphen ("- Error hint:"). For consistency with the V1 client error formatting (lines 56-59), all parts should follow the same spacing pattern.
| f" Status code: {response.status_code} - " | |
| f"Error description: {error_description} -" | |
| f" Error hint: {error_hint} - " | |
| f" validation errors: {error_validation}" | |
| f"Status code: {response.status_code} - " | |
| f"Error description: {error_description} - " | |
| f"Error hint: {error_hint} - " | |
| f"Validation errors: {error_validation}" |
|
|
||
|
|
There was a problem hiding this comment.
The V2Amount model is missing strip_whitespace validation on string fields 'currency' and 'value'. Following the codebase convention that all string fields should have strip_whitespace validation to prevent Brenger API failures, these fields should have the validator applied.
| _strip_whitespace = field_validator("currency", "value", mode="before")( | |
| strip_whitespace | |
| ) |
| class V2Event(BaseModel): | ||
| id: str | ||
| timestamp: str | ||
| status: str |
There was a problem hiding this comment.
The V2Event and V2WebhookPayload models use string types for the 'timestamp' field. This is less type-safe and makes it harder to work with dates/times programmatically. Consider using datetime type with proper validators (similar to the shipping_date validation in ShipmentCreateRequest) to ensure timestamp parsing and validation, unless the API specifically requires string format in a non-standard way.
| lng: Optional[float] = None | ||
|
|
||
| _strip_whitespace = field_validator( | ||
| "line1", "line2", "postal_code", "locality", mode="before" |
There was a problem hiding this comment.
The V2Address model is missing strip_whitespace validation on the 'country' and 'administrative_area' fields. Following the codebase convention that all string fields should have strip_whitespace validation to prevent Brenger API failures, these fields should be included in the validator along with line1, line2, postal_code, and locality.
| "line1", "line2", "postal_code", "locality", mode="before" | |
| "country", | |
| "administrative_area", | |
| "line1", | |
| "line2", | |
| "postal_code", | |
| "locality", | |
| mode="before", |
| address: V2Address | ||
|
|
||
| _strip_whitespace = field_validator( | ||
| "email", "first_name", "last_name", "company_name", mode="before" |
There was a problem hiding this comment.
The V2Stop model is missing strip_whitespace validation on the 'preferred_locale' field. Following the codebase convention that all string fields should have strip_whitespace validation to prevent Brenger API failures, this field should be included in the validator.
| "email", "first_name", "last_name", "company_name", mode="before" | |
| "email", | |
| "first_name", | |
| "last_name", | |
| "company_name", | |
| "preferred_locale", | |
| mode="before", |
|
|
||
|
|
There was a problem hiding this comment.
The V2QuoteRequest model is missing strip_whitespace validation on the 'external_reference' field. Following the codebase convention that all string fields should have strip_whitespace validation to prevent Brenger API failures, this field should have the validator applied.
| _strip_whitespace = field_validator("external_reference", mode="before")( | |
| strip_whitespace | |
| ) |
| [tool.poetry] | ||
| name = "brenger" | ||
| version = "0.1.8" | ||
| version = "2.0.0" |
There was a problem hiding this comment.
The version bump from 0.1.8 to 2.0.0 appears to be too aggressive. According to semantic versioning, a major version increment (0.x.x to 2.0.0) should only occur when there are backward-incompatible changes. Since the PR description explicitly states "Keep v1 client and models intact for backward compatibility", this suggests the changes are backward compatible. A more appropriate version would be 0.2.0 or 1.0.0 (if graduating from beta), not 2.0.0.
| version = "2.0.0" | |
| version = "0.2.0" |
brenger/models.py
Outdated
| pickup: Dict | ||
| delivery: Dict | ||
| external_reference: Optional[str] = None |
There was a problem hiding this comment.
The V2QuoteResponse model uses Dict types for 'pickup' and 'delivery' fields instead of strongly-typed models. This is inconsistent with V2ShipmentCreateResponse which uses V2Stop. Using Dict reduces type safety and makes the response structure unclear. Consider using V2Stop or a similar typed model for consistency.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rovements - Configurable timeout (default 30s) and base_url for sandbox/production - _request() wrapper catches network errors and raises APIServerError - _extract_error_details() handles non-JSON response bodies - 5xx range check instead of just status == 500 - Same error handling improvements applied to v1 client Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
BrengerV2APIClientwith endpoints: get_quote, create_shipment, get_shipment_status, cancel_shipment, get_refundTest plan
🤖 Generated with Claude Code