-
-
Notifications
You must be signed in to change notification settings - Fork 233
Removed '@' check in username check #287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Warning
|
| Cohort / File(s) | Summary |
|---|---|
Core client updatesgarminconnect/__init__.py |
Added `_validate_json_exists(response: requests.Response) -> dict[str, Any] |
Sequence Diagram(s)
sequenceDiagram
autonumber
actor User
participant GarminClient as Garmin Client
participant Auth as Auth Service
rect rgb(240,255,240)
note over GarminClient: Login flow (after change)
User->>GarminClient: login(username, password)
GarminClient->>Auth: Initiate login / MFA flow
Auth-->>GarminClient: Auth result
GarminClient-->>User: Tokens / Error
end
sequenceDiagram
autonumber
actor User
participant GarminClient as Garmin Client
participant API as Garmin API
rect rgb(245,245,255)
note over GarminClient: Weigh‑in POST handling (after change)
User->>GarminClient: add_weigh_in(...)
GarminClient->>API: POST /user/weight
API-->>GarminClient: 200/201 with JSON or 204 No Content
alt JSON body present
GarminClient->>GarminClient: _validate_json_exists -> returns JSON dict
GarminClient-->>User: dict
else 204 No Content
GarminClient->>GarminClient: _validate_json_exists -> returns None
GarminClient-->>User: None
end
end
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
I hop in code with nimble paws,
Removed the fence of "@" because—
Now logins skip that picky gate,
And weigh-ins handle empty state.
Thump-thump—small fixes, boundless cheers! 🥕🐇
Pre-merge checks and finishing touches
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes Check | The pull request introduces a new _validate_json_exists helper and updates weigh-in methods, which are unrelated to the removal of the '@' validation for login specified in issue #286. These additions concern response handling and data submission endpoints that the linked issue does not cover. Including them in this PR mixes distinct concerns and may complicate review. | Please extract the helper addition and weigh-in updates into a separate pull request and keep this PR focused solely on removing the '@' username validation. | |
| Docstring Coverage | Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title Check | ✅ Passed | The pull request title succinctly and accurately describes the primary change to remove the '@' symbol validation in the username check, directly reflecting the main fix for issue #286. It clearly references the specific validation being removed and is concise without extraneous detail. Thus it effectively communicates the intent of the PR to team members scanning history. |
| Linked Issues Check | ✅ Passed | The changes remove the '@' symbol validation from the login function as specified in issue #286. The login flow now proceeds without raising an exception for usernames lacking an '@' symbol, fulfilling the user requirement. All relevant login logic changes align with the linked issue objective. |
✨ Finishing touches
- 📝 Generate Docstrings
🧪 Generate unit tests
- Create PR with unit tests
- Post copyable unit tests in a comment
📜 Recent review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
garminconnect/__init__.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- garminconnect/init.py
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 help to get the list of available commands and usage tips.
This should fix #286 , since some usernames don't have the '@'.
A future improvement for readability and to avoid confusion could be to replace all instances of "email" mentioned to "username" since it seems to be possible for users to login without an email
Summary by CodeRabbit