refactor:move_to_extras #315
Conversation
package is too bloated for basic usage, timezonefinder, langcodes and oauth dependencies have been moved to extras
WalkthroughThis pull request introduces several changes across multiple files, primarily focusing on improving the initialization and loading of variables and imports. Key modifications include the lazy loading of the Changes
Possibly related PRs
Poem
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
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #315 +/- ##
==========================================
+ Coverage 44.74% 44.86% +0.11%
==========================================
Files 33 33
Lines 3426 3415 -11
==========================================
- Hits 1533 1532 -1
+ Misses 1893 1883 -10 ☔ 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: 2
🧹 Outside diff range and nitpick comments (5)
ovos_utils/gui.py (1)
Line range hint
1-99: Good architectural direction.This change aligns well with the PR's objective of reducing package bloat. The removal of deprecated code paths and simplification of the GUI detection logic contributes to a more maintainable codebase. The utility functions maintain clear separation of concerns and follow the single responsibility principle.
ovos_utils/lang/__init__.py (2)
Line range hint
7-19: Improve error handling for missing optional dependencyThe function handles missing
langcodesgracefully, but we should log a warning to help users understand why the fallback is being used.def standardize_lang_tag(lang_code: str, macro=True) -> str: """https://langcodes-hickford.readthedocs.io/en/sphinx/index.html""" try: from langcodes import standardize_tag as std return str(std(lang_code, macro=macro)) - except: + except ImportError: + LOG.debug("langcodes not installed, using fallback language standardization") if macro: return lang_code.split("-")[0].lower() if "-" in lang_code: a, b = lang_code.split("-", 2) return f"{a.lower()}-{b.upper()}" return lang_code.lower()🧰 Tools
🪛 Ruff (0.8.0)
13-13: Do not use bare
except(E722)
Line range hint
22-41: Enhance error handling in get_language_dirThe function should distinguish between missing dependency and invalid language code errors.
for f in listdir(base_path): if isdir(f"{base_path}/{f}"): try: from langcodes import tag_distance score = tag_distance(lang, f) - except: # not a valid language code + except ImportError: + LOG.debug("langcodes not installed, skipping language distance calculation") + return None + except Exception: # not a valid language code continue🧰 Tools
🪛 Ruff (0.8.0)
32-32: Do not use bare
except(E722)
ovos_utils/geolocation.py (2)
12-13: Add type hint to global variableRestore the type hint for better code maintainability and IDE support.
-_tz_finder = None +from typing import Optional +_tz_finder: Optional['TimezoneFinder'] = None
Line range hint
43-47: Enhance error message clarityThe error messages should be more specific about the type of error encountered.
except ValueError as e: - raise ValueError(f"Invalid coordinates: {str(e)}") from e + raise ValueError(f"Invalid coordinates (lat: {lat}, lon: {lon}): {str(e)}") from e except Exception as e: - raise RuntimeError(f"Timezone lookup failed: {str(e)}") from e + raise RuntimeError(f"Timezone lookup failed for coordinates (lat: {lat}, lon: {lon}): {str(e)}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
ovos_utils/geolocation.py(2 hunks)ovos_utils/gui.py(1 hunks)ovos_utils/lang/__init__.py(2 hunks)ovos_utils/oauth.py(1 hunks)requirements/extras.txt(1 hunks)requirements/requirements.txt(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- requirements/requirements.txt
- requirements/extras.txt
🔇 Additional comments (2)
ovos_utils/gui.py (2)
37-37: LGTM! Clean and efficient implementation.
The simplified implementation is more maintainable and removes technical debt by eliminating deprecated checks. The use of a generator expression with any() is both Pythonic and efficient.
Line range hint 31-38: Verify impact and update documentation.
While the code changes look good, please ensure:
- The removal of deprecated GUI app checks is documented in the changelog
- Users are informed about this change if they're still using deprecated GUI applications
Let's verify the usage of deprecated GUI applications in the codebase:
| from oauthlib.oauth2 import WebApplicationClient | ||
| client = WebApplicationClient(client_id, refresh_token=refresh_token) | ||
| uri, headers, body = client.prepare_refresh_token_request(token_endpoint) | ||
| refresh_result = requests.post(uri, headers=headers, data=body, |
There was a problem hiding this comment.
Add error handling for missing optional dependency
The function should gracefully handle missing oauthlib dependency and provide clear feedback to users.
- from oauthlib.oauth2 import WebApplicationClient
- client = WebApplicationClient(client_id, refresh_token=refresh_token)
+ try:
+ from oauthlib.oauth2 import WebApplicationClient
+ client = WebApplicationClient(client_id, refresh_token=refresh_token)
+ except ImportError:
+ LOG.error("oauthlib not installed. Please install 'ovos-utils[oauth]' to use OAuth functionality")
+ return None📝 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.
| from oauthlib.oauth2 import WebApplicationClient | |
| client = WebApplicationClient(client_id, refresh_token=refresh_token) | |
| uri, headers, body = client.prepare_refresh_token_request(token_endpoint) | |
| refresh_result = requests.post(uri, headers=headers, data=body, | |
| try: | |
| from oauthlib.oauth2 import WebApplicationClient | |
| client = WebApplicationClient(client_id, refresh_token=refresh_token) | |
| except ImportError: | |
| LOG.error("oauthlib not installed. Please install 'ovos-utils[oauth]' to use OAuth functionality") | |
| return None | |
| uri, headers, body = client.prepare_refresh_token_request(token_endpoint) | |
| refresh_result = requests.post(uri, headers=headers, data=body, |
| if _tz_finder is None: | ||
| from timezonefinder import TimezoneFinder | ||
| # lazy loaded, resource intensive so we only want to do it once | ||
| _tz_finder = TimezoneFinder() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for missing optional dependency
The function should provide clear feedback when timezonefinder is not installed.
if _tz_finder is None:
- from timezonefinder import TimezoneFinder
- # lazy loaded, resource intensive so we only want to do it once
- _tz_finder = TimezoneFinder()
+ try:
+ from timezonefinder import TimezoneFinder
+ # lazy loaded, resource intensive so we only want to do it once
+ _tz_finder = TimezoneFinder()
+ except ImportError:
+ LOG.error("timezonefinder not installed. Please install 'ovos-utils[location]' to use timezone functionality")
+ raise RuntimeError("timezonefinder package is required for timezone lookup")📝 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.
| if _tz_finder is None: | |
| from timezonefinder import TimezoneFinder | |
| # lazy loaded, resource intensive so we only want to do it once | |
| _tz_finder = TimezoneFinder() | |
| if _tz_finder is None: | |
| try: | |
| from timezonefinder import TimezoneFinder | |
| # lazy loaded, resource intensive so we only want to do it once | |
| _tz_finder = TimezoneFinder() | |
| except ImportError: | |
| LOG.error("timezonefinder not installed. Please install 'ovos-utils[location]' to use timezone functionality") | |
| raise RuntimeError("timezonefinder package is required for timezone lookup") |
package is too bloated for basic usage, timezonefinder, langcodes and oauth dependencies have been moved to extras
Summary by CodeRabbit
New Features
langcodes,timezonefinder, andoauthlibto enhance functionality.orjsonpackage for improved performance.Bug Fixes
get_timezoneandstandardize_lang_tagfunctions for better user feedback.Refactor
is_gui_runningfunction for clearer application status checks.