remove black and isort. use only ruff #77
Conversation
WalkthroughThis pull request includes multiple changes across several files, primarily focusing on updates to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #77 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 34 34
Lines 1287 1287
=========================================
Hits 1287 1287
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range comments (10)
garth/stats/_base.py (1)
Line range hint
20-52: Add error handling and documentation for complex pagination logicThe list method implementation has several areas for improvement:
- Missing docstring explaining the pagination logic
- No error handling for API failures
- Potential stack overflow risk for very large periods
Consider adding error handling and documentation:
@classmethod def list( cls, end: Union[date, str, None] = None, period: int = 1, *, client: Optional[http.Client] = None, ) -> List["Stats"]: + """Retrieve stats for a specified date range with pagination support. + + Args: + end: The end date for stats retrieval. Defaults to current date. + period: Number of days/weeks to retrieve. Defaults to 1. + client: Optional HTTP client. Defaults to global client. + + Returns: + List of Stats objects for the specified period. + + Raises: + ValueError: If period is negative or zero. + HTTPError: If the API request fails. + """ + if period <= 0: + raise ValueError("Period must be positive") + client = client or http.client end = format_end_date(end) period_type = "days" if "daily" in cls._path else "weeks" if period > cls._page_size: page = cls.list(end, cls._page_size, client=client) if not page: return [] page = ( cls.list( end - timedelta(**{period_type: cls._page_size}), period - cls._page_size, client=client, ) + page ) return page start = end - timedelta(**{period_type: period - 1}) path = cls._path.format(start=start, end=end, period=period) - page_dirs = client.connectapi(path) + try: + page_dirs = client.connectapi(path) + except Exception as e: + raise HTTPError(f"Failed to fetch stats: {e}") from eConsider implementing an iterative approach instead of recursion for handling large periods to avoid potential stack overflow:
def list(cls, end: Union[date, str, None] = None, period: int = 1, *, client: Optional[http.Client] = None) -> List["Stats"]: results = [] current_end = format_end_date(end) remaining_period = period while remaining_period > 0: chunk_size = min(remaining_period, cls._page_size) chunk = cls._fetch_chunk(current_end, chunk_size, client) if not chunk: break results = chunk + results remaining_period -= chunk_size current_end -= timedelta(**{period_type: chunk_size}) return resultsgarth/utils.py (2)
Line range hint
7-27: Add pattern documentation and input validationThe camelCase conversion implementation could be improved with:
- Documentation explaining the regex pattern
- Input validation
- Edge case handling
CAMEL_TO_SNAKE = re.compile( r"((?<=[a-z0-9])[A-Z]|(?!^)[A-Z](?=[a-z])|(?<=[a-zA-Z])[0-9])" ) def camel_to_snake(camel_str: str) -> str: + """Convert a camelCase string to snake_case. + + Pattern explanation: + - (?<=[a-z0-9])[A-Z]: matches capital letters preceded by lowercase/numbers + - (?!^)[A-Z](?=[a-z]): matches capitals followed by lowercase, except at start + - (?<=[a-zA-Z])[0-9]: matches numbers preceded by letters + + Args: + camel_str: String in camelCase format + + Returns: + String in snake_case format + + Raises: + TypeError: If input is not a string + """ + if not isinstance(camel_str, str): + raise TypeError("Input must be a string") + if not camel_str: + return camel_str snake_str = CAMEL_TO_SNAKE.sub(r"_\1", camel_str) return snake_str.lower()
Line range hint
49-67: Add type hints and cycle detectionThe
asdictfunction could be improved with:
- Type hints for better type safety
- Cycle detection to prevent infinite recursion
- Optional performance optimization for large objects
-def asdict(obj): +def asdict(obj: Any, _seen: Optional[Set[int]] = None) -> Any: + """Convert an object to a dictionary recursively. + + Args: + obj: Object to convert + _seen: Set of object ids already processed (for cycle detection) + + Returns: + Dictionary representation of the object + """ + if _seen is None: + _seen = set() + + # Handle cycles + obj_id = id(obj) + if obj_id in _seen: + return "<recursive>" + _seen.add(obj_id) + if dataclasses.is_dataclass(obj): result = {} for field in dataclasses.fields(obj): value = getattr(obj, field.name) - result[field.name] = asdict(value) + result[field.name] = asdict(value, _seen) return result if isinstance(obj, List): - return [asdict(v) for v in obj] + return [asdict(v, _seen) for v in obj] if isinstance(obj, (datetime, date)): return obj.isoformat() return objgarth/sso.py (6)
Line range hint
15-19: Potential Thread Safety Issue with Global VariableOAUTH_CONSUMERThe use of the global variable
OAUTH_CONSUMERwithout proper synchronization could lead to race conditions in a multithreaded environment. If multiple threads initializeGarminOAuth1Sessionsimultaneously, they might attempt to fetch and setOAUTH_CONSUMERat the same time.Consider using a thread lock to ensure that
OAUTH_CONSUMERis initialized safely:import threading _OAUTH_CONSUMER_LOCK = threading.Lock() OAUTH_CONSUMER: Dict[str, str] = {} class GarminOAuth1Session(OAuth1Session): def __init__( self, /, parent: Optional[Session] = None, **kwargs, ): global OAUTH_CONSUMER with _OAUTH_CONSUMER_LOCK: if not OAUTH_CONSUMER: OAUTH_CONSUMER = requests.get(OAUTH_CONSUMER_URL).json() super().__init__( OAUTH_CONSUMER["consumer_key"], OAUTH_CONSUMER["consumer_secret"], **kwargs, ) # Rest of the code...
Line range hint
23-25: Avoid Using Mutable Default Argument forprompt_mfaUsing a lambda function as a default value for
prompt_mfacan lead to unexpected behavior, especially when the default value is mutable or stateful.Refactor the function to set
prompt_mfatoNoneby default and assign the lambda inside the function if needed:def login( email: str, password: str, /, client: Optional["http.Client"] = None, - prompt_mfa: Callable = lambda: input("MFA code: "), + prompt_mfa: Optional[Callable] = None, ) -> Tuple[OAuth1Token, OAuth2Token]: + if prompt_mfa is None: + prompt_mfa = lambda: input("MFA code: ") client = client or http.client # Rest of the code...
Line range hint
67-72:asyncio.runMay Not Work Inside Existing Event LoopCalling
asyncio.runinsidehandle_mfacould raise aRuntimeErrorif there's already an event loop running (e.g., in an async context).Modify
handle_mfato handle asynchronousprompt_mfafunctions without causing runtime errors:import sys def handle_mfa( client: "http.Client", signin_params: dict, prompt_mfa: Callable ) -> None: csrf_token = get_csrf_token(client.last_resp.text) if asyncio.iscoroutinefunction(prompt_mfa): if sys.version_info >= (3, 7) and asyncio.get_running_loop(): mfa_code = asyncio.run(prompt_mfa()) else: mfa_code = asyncio.run(prompt_mfa()) else: mfa_code = prompt_mfa() # Rest of the code...Alternatively, consider making
handle_mfaan asynchronous function or requiringprompt_mfato be synchronous.
Line range hint
90-93: Use Explicit Exception Handling Instead of AssertionsUsing
assertstatements for runtime checks is not recommended in production code because assertions can be disabled with optimization flags, potentially bypassing critical checks.Replace
assertstatements with explicit exceptions:m = re.search(r'embed\?ticket=([^"]+)"', client.last_resp.text) -assert m +if not m: + raise GarthException("Ticket not found in response") ticket = m.group(1)Apply similar changes to other
assertstatements to ensure checks are always enforced.
Line range hint
116-121: Handle Missing Keys inset_expirationsFunctionAccessing
token["expires_in"]andtoken["refresh_token_expires_in"]without validation could raise aKeyErrorif they are missing from thetokendictionary.Add error handling to verify the presence of these keys:
def set_expirations(token: dict) -> dict: required_keys = ["expires_in", "refresh_token_expires_in"] for key in required_keys: if key not in token: raise GarthException(f"Missing '{key}' in token") token["expires_at"] = int(time.time() + token["expires_in"]) token["refresh_token_expires_at"] = int( time.time() + token["refresh_token_expires_in"] ) return token
Line range hint
128-135: Provide More Descriptive Error MessagesThe exception messages in
get_csrf_tokenandget_titleare generic. Providing detailed messages can help with debugging.Enhance the exception messages to include more context:
def get_csrf_token(html: str) -> str: m = CSRF_RE.search(html) if not m: - raise GarthException("Couldn't find CSRF token") + raise GarthException("CSRF token not found in the HTML response") return m.group(1) def get_title(html: str) -> str: m = TITLE_RE.search(html) if not m: - raise GarthException("Couldn't find title") + raise GarthException("Title tag not found in the HTML response") return m.group(1)garth/data/_base.py (1)
Line range hint
10-22: Ensure Proper Default Values and Parameter OrderThe
listmethod now includesmax_workersas a parameter. Ensure that parameters are ordered logically, and default values are appropriate.Consider reordering parameters for clarity, placing
clientandmax_workersafter essential parameters:def list( cls, end: Union[date, str, None] = None, days: int = 1, + max_workers: int = MAX_WORKERS, *, client: Optional[http.Client] = None, - max_workers: int = MAX_WORKERS, ):
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Makefile(1 hunks)garth/__init__.py(1 hunks)garth/data/_base.py(1 hunks)garth/http.py(1 hunks)garth/sso.py(1 hunks)garth/stats/_base.py(1 hunks)garth/stats/intensity_minutes.py(1 hunks)garth/stats/steps.py(1 hunks)garth/stats/stress.py(1 hunks)garth/users/__init__.py(1 hunks)garth/utils.py(1 hunks)pyproject.toml(1 hunks)tests/test_http.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/test_http.py (1)
Pattern tests/**: - test functions shouldn't have a return type hint
- it's ok to use
assertinstead ofpytest.assume()
🔇 Additional comments (8)
garth/stats/stress.py (1)
7-9: Clarify PR scope and title.
The PR title "remove black and isort. use only ruff" doesn't match the actual changes, which focus on centralizing API path definitions. Consider:
- Updating the PR title to reflect the actual changes
- Splitting these changes into a separate PR if they're unrelated to the linting tools transition
Let's verify if there are any formatting-related changes in the PR:
garth/stats/_base.py (1)
Line range hint 14-18: LGTM!
The class structure is well-designed with appropriate use of frozen dataclass and ClassVar for class-level constants.
garth/utils.py (1)
Line range hint 37-47: LGTM!
The date handling utilities are well-implemented with proper type hints and error handling.
garth/users/__init__.py (1)
4-4: Addition of __all__ Declaration Improves Module Encapsulation
The introduction of __all__ = ["UserProfile", "UserSettings"] explicitly defines the public interface of the module, enhancing code readability and maintenance.
garth/__init__.py (1)
Line range hint 16-38: Expansion of Public API via __all__
Adding functions like configure, connectapi, download, login, resume, save, and upload to __all__ makes them part of the public interface, improving accessibility for users.
Direct Assignment of Functions Enhances Usability
Assigning functions directly from client simplifies the import process for users, making the API more user-friendly.
Makefile (1)
25-26: LGTM! Successfully transitioned to ruff for formatting and linting.
The changes correctly replace black/isort with ruff commands while maintaining equivalent functionality:
- Format target:
ruff formatfor formatting,ruff check --fixfor auto-fixes - Lint target:
ruff format --checkfor format checking,ruff checkfor linting
Let's verify the ruff configuration:
Also applies to: 30-31
✅ Verification successful
Ruff configuration looks good and aligns with the formatting transition
The pyproject.toml contains proper ruff configuration that matches the Makefile changes:
- Line length and indentation settings are defined
- Lint section includes essential rules (E, F, I) covering both formatting and import sorting
- Format section specifies double quotes style
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check ruff configuration in pyproject.toml
# Verify ruff configuration exists and includes necessary settings
rg -A 10 '^\[tool\.ruff\]' pyproject.toml
Length of output: 212
tests/test_http.py (1)
186-186: LGTM! Case change in hex literal is a benign formatting update.
The change from \x4B to \x4b is equivalent in Python and likely results from ruff's formatting rules.
garth/http.py (1)
15-15: LGTM! Added blank line improves readability.
The additional blank line follows standard Python formatting practices.
|
|
||
| BASE_PATH = "/usersummary-service/stats/im" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider documenting the "/im" abbreviation.
While centralizing the base path is good, the "/im" abbreviation in the path might not be immediately clear to other developers.
Consider adding a brief comment explaining that "im" stands for "intensity minutes":
+# Base path for intensity minutes endpoints
BASE_PATH = "/usersummary-service/stats/im"📝 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.
| BASE_PATH = "/usersummary-service/stats/im" | |
| # Base path for intensity minutes endpoints | |
| BASE_PATH = "/usersummary-service/stats/im" |
|
|
||
| BASE_PATH = "/usersummary-service/stats/stress" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider centralizing all base paths.
The changes are consistent with the pattern in other files. Since multiple stats files are defining their base paths, consider creating a shared constants file (e.g., garth/stats/constants.py) to centralize all these path definitions in one place.
Example structure:
# garth/stats/constants.py
STEPS_BASE_PATH = "/usersummary-service/stats/steps"
INTENSITY_MINUTES_BASE_PATH = "/usersummary-service/stats/im"
STRESS_BASE_PATH = "/usersummary-service/stats/stress"|
|
||
| BASE_PATH = "/usersummary-service/stats/stress" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding type annotation and moving to constants file
The BASE_PATH constant would benefit from:
- Type annotation for better type safety
- Potentially being moved to a central constants file if other modules need similar API paths
-BASE_PATH = "/usersummary-service/stats/stress"
+BASE_PATH: str = "/usersummary-service/stats/stress"📝 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.
| BASE_PATH = "/usersummary-service/stats/stress" | |
| BASE_PATH: str = "/usersummary-service/stats/stress" | |
| @@ -5,6 +5,7 @@ | |||
| from .. import http | |||
| from ..utils import date_range, format_end_date | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider Making MAX_WORKERS Configurable
Defining MAX_WORKERS as a constant set to 10 might not be optimal for all environments. Systems with different resources may benefit from adjusting this value.
Allow users to configure MAX_WORKERS or set a dynamic default based on the number of available CPU cores:
import os
MAX_WORKERS = min(32, (os.cpu_count() or 1) + 4)| [tool.ruff] | ||
| line-length = 79 | ||
| indent-width = 4 | ||
| target-version = "py310" | ||
|
|
||
| [tool.ruff.lint] | ||
| select = ["E", "F", "I"] | ||
| ignore = [] | ||
|
|
||
| [tool.ruff.format] | ||
| quote-style = "double" | ||
| indent-style = "space" | ||
| skip-magic-trailing-comma = false | ||
| line-ending = "auto" | ||
|
|
||
| [tool.ruff.lint.isort] | ||
| known-first-party = ["garth"] | ||
| combine-as-imports = true | ||
| lines-after-imports = 2 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Transition to ruff Simplifies Linting and Formatting Configuration
The switch from black and isort to ruff consolidates linting and formatting tools, potentially improving maintenance and performance.
Ensure Consistency in ruff Configuration Sections
Some ruff configurations are nested under [tool.ruff.lint.isort] despite isort being removed. This might cause confusion.
Consider updating the configuration sections for clarity:
[tool.ruff]
line-length = 79
indent-width = 4
target-version = "py310"
[tool.ruff.lint]
select = ["E", "F", "I"]
ignore = []
[tool.ruff.format]
quote-style = "double"
indent-style = "space"
skip-magic-trailing-comma = false
line-ending = "auto"
-[tool.ruff.lint.isort]
+[tool.ruff.isort]
known-first-party = ["garth"]
combine-as-imports = true
lines-after-imports = 2This adjustment aligns the configuration with ruff's expected format.
Committable suggestion skipped: line range outside the PR's diff.
| @@ -12,6 +12,7 @@ | |||
| from .exc import GarthHTTPError | |||
| from .utils import asdict | |||
|
|
|||
|
|
|||
| USER_AGENT = {"User-Agent": "GCM-iOS-5.7.2.1"} | |||
|
|
|||
There was a problem hiding this comment.
💡 Codebase verification
Unrelated USER_AGENT constant addition found in multiple files
The search results confirm that there are two different USER_AGENT constants defined and used in the codebase:
garth/http.py: iOS user agent string used in HTTP session headersgarth/sso.py: Android user agent string used in SSO-related requests
This indicates that the addition of USER_AGENT constant is indeed a functional change rather than a formatting one, and thus unrelated to the PR's stated objective of transitioning from black/isort to ruff.
🔗 Analysis chain
Note: USER_AGENT constant addition is unrelated to formatting changes.
While this change is valid, it appears to be unrelated to the PR's objective of transitioning from black/isort to ruff.
Let's check if this constant is used elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of USER_AGENT constant
# Search for USER_AGENT references
rg "USER_AGENT" --type py
Length of output: 307
Summary by CodeRabbit
Release Notes
New Features
configure,connectapi,download,login,resume,save, andupload.Clientclass with methods for file uploads and improved session management.Improvements
listmethod in theDataclass to allow specification of maximum worker threads for data fetching.Bug Fixes
Chores
blackandisorttoruff.