Open
Conversation
Owner
|
@coderabbitai full review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #196 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 68 70 +2
Lines 3508 3651 +143
==========================================
+ Hits 3508 3651 +143
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Author
|
@coderabbitai full review |
Owner
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
|
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:
WalkthroughAdds a new immutable Badge data model with constants, computed properties, and API methods (get/list/reload); exposes Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test / Caller
participant Badge as Badge (model)
participant HTTP as HTTP Client
participant API as Badge Service
Test->>Badge: call Badge.list() or Badge.get(badge_id)
Badge->>HTTP: GET /badge-service/...
HTTP->>API: send request
API-->>HTTP: JSON response (camelCase)
HTTP-->>Badge: return payload
Badge->>Badge: camelCase→snake_case, validate, instantiate
Badge-->>Test: return Badge instance(s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/garth/data/badge.py (1)
105-106: Fix typo in public property name before API surface hardens.
month_chalengeis misspelled. Since this is a new API, addingmonth_challengenow (and optionally keeping the old name as alias) avoids long-term public API friction.✏️ Proposed rename with compatibility alias
- def month_chalenge(self) -> bool: + def month_challenge(self) -> bool: return self.badge_category_id == Badge.CATEGORY_CHALLENGES and self.limited_time + + `@property` + def month_chalenge(self) -> bool: + return self.month_challenge🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/garth/data/badge.py` around lines 105 - 106, Rename the misspelled public property month_chalenge to month_challenge on the Badge class and update its implementation to return the same boolean expression (self.badge_category_id == Badge.CATEGORY_CHALLENGES and self.limited_time); to avoid breaking callers, add month_chalenge as a thin compatibility alias that delegates to month_challenge (and optionally mark it as deprecated in a docstring or comment) so both names work for now while new code uses month_challenge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/garth/data/badge.py`:
- Line 133: The formatting error is caused by a style drift on the call to
client.connectapi in the badge module; run the repository formatter (ruff format
or make format) and commit the changes so the line "available =
client.connectapi(...)" matches project formatting rules (ensure spacing and
line wrapping are corrected). Locate the usage of client.connectapi and the
available assignment in src/garth/data/badge.py and reformat the file (or apply
ruff format) to resolve the CI failure.
- Around line 121-123: The code calls client.connectapi(...) then immediately
runs camel_to_snake_dict and asserts types, which will raise unclear errors if
connectapi returns None or an unexpected shape; update the flow around
client.connectapi, camel_to_snake_dict, and the surrounding functions in
badge.py to validate the payload first (check for None and ensure
isinstance(data, dict) for detail endpoints and isinstance(data, list) for list
endpoints), raise a clear ValueError or custom exception with context (including
the endpoint path and received type) when the payload is invalid, and only then
call camel_to_snake_dict (or map camel_to_snake_dict over list items) so
transformations are only applied to validated types.
In `@tests/data/test_badge.py`:
- Line 19: Replace all assertions in tests/data/test_badge.py that compare
boolean values using "== True" or "== False" with direct truthiness checks to
satisfy Ruff E712; e.g., change "assert earned_badge.repeatable == True" to
"assert earned_badge.repeatable" and any "== False" checks to "assert not
<expr>". Update each occurrence (including assertions referencing
earned_badge.repeatable and the other boolean attributes flagged at lines 25,
28, 31, 35, 37) accordingly so the tests use plain truthy/falsey assertions.
---
Nitpick comments:
In `@src/garth/data/badge.py`:
- Around line 105-106: Rename the misspelled public property month_chalenge to
month_challenge on the Badge class and update its implementation to return the
same boolean expression (self.badge_category_id == Badge.CATEGORY_CHALLENGES and
self.limited_time); to avoid breaking callers, add month_chalenge as a thin
compatibility alias that delegates to month_challenge (and optionally mark it as
deprecated in a docstring or comment) so both names work for now while new code
uses month_challenge.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/data/cassettes/test_badge_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_badge_list.yamlis excluded by!tests/**/cassettes/**
📒 Files selected for processing (4)
src/garth/__init__.pysrc/garth/data/__init__.pysrc/garth/data/badge.pytests/data/test_badge.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/garth/data/badge.py (6)
31-31: Remove unnecessary string quote aroundint.
builtins.list["int"]uses a forward reference string, butintis a builtin that's always available—no forward reference is needed. The use ofbuiltins.listitself is fine to avoid shadowing by thelistclassmethod.✏️ Suggested change
- badge_type_ids: builtins.list["int"] + badge_type_ids: builtins.list[int]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/garth/data/badge.py` at line 31, The type annotation for badge_type_ids uses an unnecessary forward-ref string; update the annotation in badge_type_ids from builtins.list["int"] to builtins.list[int] so it uses the actual int type (leave the use of builtins.list intact to avoid shadowing).
135-135: Parameteridshadows the built-in function.Consider renaming to
badge_idto avoid shadowing Python's built-inid().✏️ Suggested change
`@classmethod` - def get(cls, id: int, client: http.Client | None = None) -> Self: + def get(cls, badge_id: int, client: http.Client | None = None) -> Self: """Get badge by ID. Args: - id: The Garmin badge ID + badge_id: The Garmin badge ID client: Optional HTTP client (uses default if not provided) Returns: Badge instance with full details """ client = client or http.client - path = f"/badge-service/badge/detail/v2/{id}" + path = f"/badge-service/badge/detail/v2/{badge_id}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/garth/data/badge.py` at line 135, The get method's parameter name id shadows Python's built-in id(); rename the parameter to badge_id in the classmethod definition (def get(cls, badge_id: int, client: http.Client | None = None) -> Self) and update all references inside the method body (and any internal calls like cls(...), queries, or variable uses) to use badge_id; also update any external call sites that pass or reference this positional/keyword argument to use the new name to keep behavior identical.
183-187: Simplify with list comprehension.The loop pattern can be condensed into a more Pythonic list comprehension.
✏️ Suggested change
- badges = [] - for item in data: - item = camel_to_snake_dict(item) - badges.append(cls(**item)) - return badges + return [cls(**camel_to_snake_dict(item)) for item in data]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/garth/data/badge.py` around lines 183 - 187, Replace the explicit loop that builds badges with a list comprehension: where the code creates badges = [] then iterates over data, calling camel_to_snake_dict(item) and badges.append(cls(**item)), change it to directly return a list comprehension that maps each item through camel_to_snake_dict and constructs cls(**item) (i.e., eliminate the temporary badges list and use [cls(**camel_to_snake_dict(item)) for item in data]); update the method containing this logic so its return is the comprehension.
128-132: Add return type annotation for consistency.The
reloadmethod lacks a return type annotation, unlikegetandlistwhich properly declare their return types.✏️ Suggested change
- def reload(self, client: http.Client | None = None): + def reload(self, client: http.Client | None = None) -> Self: """Get actual data for Badge Useful to retrieve actual information for repeatable badges from list response """ return Badge.get(self.badge_id, client or http.client)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/garth/data/badge.py` around lines 128 - 132, Add a return type annotation to the reload method to match the return type of Badge.get (so the signature mirrors Badge.get's declared return type); update def reload(self, client: http.Client | None = None) to explicitly return the same type as Badge.get (reference Badge.get and self.badge_id and http.client) so the method signature is consistent with get and list.
171-179: Replaceassertwith explicit exceptions and validate list items.Same concern as in
get:assertis disabled with-O. Additionally, the code doesn't validate that each item in the lists is adictbefore passing tocamel_to_snake_dict, which could cause unclear errors if the API returns unexpected shapes.🛡️ Proposed fix
path = "/badge-service/badge/earned" earned = client.connectapi(path) - assert isinstance(earned, list), ( - f"Expected list from {path}, got {type(earned).__name__}" - ) + if not isinstance(earned, list): + raise TypeError(f"Expected list from {path}, got {type(earned).__name__}") path = "/badge-service/badge/available?showExclusiveBadge=true" available = client.connectapi(path) - assert isinstance(available, list), ( - f"Expected list from {path}, got {type(available).__name__}" - ) + if not isinstance(available, list): + raise TypeError(f"Expected list from {path}, got {type(available).__name__}") data = earned + available + if not all(isinstance(item, dict) for item in data): + raise TypeError("Badge list payload contains non-dict entries") badges = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/garth/data/badge.py` around lines 171 - 179, Replace the runtime-only asserts in the block that fetches "earned" and "available" with explicit checks that raise informative exceptions (TypeError/ValueError) when the API returns non-list values; after confirming each value is a list, iterate over earned and available and validate that every item is a dict before calling camel_to_snake_dict (raise a TypeError with context including the path and offending index/type if not a dict). Locate the variables/paths named earned, available and the client.connectapi calls and update those checks to provide clear error messages rather than using assert.
148-151: Consider using explicit exceptions instead ofassertfor input validation.
assertstatements are removed when Python runs with optimization flags (-Oor-OO), which could allow invalid data through in production. The previous review suggested raisingTypeErrorexplicitly.🛡️ Proposed fix
data = client.connectapi(path) - assert data, f"No data returned from {path}" - assert isinstance(data, dict), ( - f"Expected dict from {path}, got {type(data).__name__}" - ) + if not data: + raise ValueError(f"No data returned from {path}") + if not isinstance(data, dict): + raise TypeError(f"Expected dict from {path}, got {type(data).__name__}") data = camel_to_snake_dict(data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/garth/data/badge.py` around lines 148 - 151, Replace the two assert statements that validate `data` and its type with explicit exceptions so validation cannot be skipped under Python optimizations: check if `data` is falsy and raise a ValueError (including `path` in the message), and replace the isinstance check with an explicit type check that raises a TypeError (also quoting `path` and the actual type name) — locate and update the block referencing `data` and `path` in src/garth/data/badge.py to use these raises instead of `assert`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/garth/data/badge.py`:
- Around line 17-22: The docstring example incorrectly chains `.annual` after
`Badge.get(...)` even though `annual` is a bool property; update the example to
first assign the Badge instance (via Badge.get) to a variable and then access
its properties separately (e.g., call Badge.get(...) into `badge`, then use
`badge.annual` and `badge.badge_name` as separate expressions) so that you never
call `.badge_name` on a bool; reference the Badge.get call, the annual property,
and the badge_name/earned_by_me attributes when making the change.
---
Nitpick comments:
In `@src/garth/data/badge.py`:
- Line 31: The type annotation for badge_type_ids uses an unnecessary
forward-ref string; update the annotation in badge_type_ids from
builtins.list["int"] to builtins.list[int] so it uses the actual int type (leave
the use of builtins.list intact to avoid shadowing).
- Line 135: The get method's parameter name id shadows Python's built-in id();
rename the parameter to badge_id in the classmethod definition (def get(cls,
badge_id: int, client: http.Client | None = None) -> Self) and update all
references inside the method body (and any internal calls like cls(...),
queries, or variable uses) to use badge_id; also update any external call sites
that pass or reference this positional/keyword argument to use the new name to
keep behavior identical.
- Around line 183-187: Replace the explicit loop that builds badges with a list
comprehension: where the code creates badges = [] then iterates over data,
calling camel_to_snake_dict(item) and badges.append(cls(**item)), change it to
directly return a list comprehension that maps each item through
camel_to_snake_dict and constructs cls(**item) (i.e., eliminate the temporary
badges list and use [cls(**camel_to_snake_dict(item)) for item in data]); update
the method containing this logic so its return is the comprehension.
- Around line 128-132: Add a return type annotation to the reload method to
match the return type of Badge.get (so the signature mirrors Badge.get's
declared return type); update def reload(self, client: http.Client | None =
None) to explicitly return the same type as Badge.get (reference Badge.get and
self.badge_id and http.client) so the method signature is consistent with get
and list.
- Around line 171-179: Replace the runtime-only asserts in the block that
fetches "earned" and "available" with explicit checks that raise informative
exceptions (TypeError/ValueError) when the API returns non-list values; after
confirming each value is a list, iterate over earned and available and validate
that every item is a dict before calling camel_to_snake_dict (raise a TypeError
with context including the path and offending index/type if not a dict). Locate
the variables/paths named earned, available and the client.connectapi calls and
update those checks to provide clear error messages rather than using assert.
- Around line 148-151: Replace the two assert statements that validate `data`
and its type with explicit exceptions so validation cannot be skipped under
Python optimizations: check if `data` is falsy and raise a ValueError (including
`path` in the message), and replace the isinstance check with an explicit type
check that raises a TypeError (also quoting `path` and the actual type name) —
locate and update the block referencing `data` and `path` in
src/garth/data/badge.py to use these raises instead of `assert`.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/garth/data/badge.pytests/data/test_badge.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/data/test_badge.py
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/garth/data/badge.py (1)
175-177: Optional: centralize payload-type error creation.There’s repeated type-check/error-message logic in
list(and similarly inget). A tiny helper would reduce duplication and also addresses the TRY003-style concern from static analysis.♻️ Refactor sketch
+ `@staticmethod` + def _require_type(payload: object, expected: type, path: str) -> None: + if not isinstance(payload, expected): + raise TypeError( + f"Expected {expected.__name__} from {path}, got {type(payload).__name__}" + ) @@ - if not isinstance(earned, list): - raise TypeError( - f"Expected list from {path}, got {type(earned).__name__}" - ) + cls._require_type(earned, list, path) @@ - if not isinstance(available, list): - raise TypeError( - f"Expected list from {path}, got {type(available).__name__}" - ) + cls._require_type(available, list, path)Also applies to: 182-184, 188-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/garth/data/badge.py` around lines 175 - 177, The repeated type-check and error-message logic in badge.py (inside the list and get functions where variables like earned and path are validated) should be centralized: add a small helper function (e.g., validate_payload_type or _raise_payload_type_error) that accepts the value, expected type name (e.g., "list"), and path and either returns nothing or raises the TypeError with the standardized message, then replace the inline checks in list and get (and any other occurrences around the earned/path checks at the referenced spots) with calls to that helper to remove duplication and satisfy static-analysis concerns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/garth/data/badge.py`:
- Around line 148-149: The current check in get uses a truthiness test that
treats empty payloads as missing; change the conditional in the get function to
explicitly check for None (i.e., if data is None) so only true None responses
from connectapi trigger the ValueError and empty/falsey payloads are allowed
through; update the raise message usage of path and variable data remains the
same.
---
Nitpick comments:
In `@src/garth/data/badge.py`:
- Around line 175-177: The repeated type-check and error-message logic in
badge.py (inside the list and get functions where variables like earned and path
are validated) should be centralized: add a small helper function (e.g.,
validate_payload_type or _raise_payload_type_error) that accepts the value,
expected type name (e.g., "list"), and path and either returns nothing or raises
the TypeError with the standardized message, then replace the inline checks in
list and get (and any other occurrences around the earned/path checks at the
referenced spots) with calls to that helper to remove duplication and satisfy
static-analysis concerns.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/garth/data/badge.py (1)
31-31: Use an immutable container forbadge_type_ids.Line 31 makes the frozen model only shallowly immutable: callers can still mutate
badge_type_idsin place and changelimited_time/repeatableafter construction.tuple[int, ...]would preserve the immutability implied by@dataclass(frozen=True).♻️ Proposed change
- badge_type_ids: builtins.list[int] + badge_type_ids: tuple[int, ...]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/garth/data/badge.py` at line 31, The Badge dataclass is only shallowly immutable because badge_type_ids is typed as list[int] and mutable; change badge_type_ids to use an immutable tuple type (tuple[int, ...]) and provide an immutable default (e.g., field(default_factory=tuple)) in the dataclass declaration, and then ensure any construction sites that currently pass a list convert to tuple(badge_type_ids) so callers can't mutate badge_type_ids after construction; also audit places that modify limited_time or repeatable to ensure they are not mutated post-construction (they should remain immutable as intended by `@dataclass`(frozen=True)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/garth/data/badge.py`:
- Around line 128-132: The reload method currently calls Badge.get(...) which
forces the return to be a base Badge and breaks the annotated -> Self; change
the call to dispatch on the runtime class (use type(self).get(...)) so
subclasses reload into their own type, preserving the Self return; update the
call site in reload to use type(self).get(self.badge_id, client or http.client).
---
Nitpick comments:
In `@src/garth/data/badge.py`:
- Line 31: The Badge dataclass is only shallowly immutable because
badge_type_ids is typed as list[int] and mutable; change badge_type_ids to use
an immutable tuple type (tuple[int, ...]) and provide an immutable default
(e.g., field(default_factory=tuple)) in the dataclass declaration, and then
ensure any construction sites that currently pass a list convert to
tuple(badge_type_ids) so callers can't mutate badge_type_ids after construction;
also audit places that modify limited_time or repeatable to ensure they are not
mutated post-construction (they should remain immutable as intended by
`@dataclass`(frozen=True)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f60cd31-4830-418b-ad8a-5b77a0e81c54
📒 Files selected for processing (1)
src/garth/data/badge.py
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
2 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.
Add Badge service support
Summary
Usage
Summary by CodeRabbit
New Features
Tests