Use timeout with int instead of num_sec#309
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideStandardizes timeout handling across widgetastic by switching string-based timeouts and the deprecated num_sec argument to integer-based timeout parameters, aligning with the updated wait_for API and deprecation plan. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider updating the method signatures with explicit type hints for the
timeout/wait_widgetparameters (e.g.,Union[int, float, timedelta]) to reflect the new accepted types and improve static checking. - If any existing callers may still pass values like
'10s', you might want to add a small compatibility layer (e.g., accepting both strings and numeric and normalizing) to avoid silent breakage when moving from"10s"to numeric defaults. - Now that
timeoutis numeric rather than a duration string, it may be helpful to ensure all internal uses treat it consistently as seconds and avoid any remaining assumptions about string-based time parsing in the called utilities (likewait_for).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider updating the method signatures with explicit type hints for the `timeout`/`wait_widget` parameters (e.g., `Union[int, float, timedelta]`) to reflect the new accepted types and improve static checking.
- If any existing callers may still pass values like `'10s'`, you might want to add a small compatibility layer (e.g., accepting both strings and numeric and normalizing) to avoid silent breakage when moving from `"10s"` to numeric defaults.
- Now that `timeout` is numeric rather than a duration string, it may be helpful to ensure all internal uses treat it consistently as seconds and avoid any remaining assumptions about string-based time parsing in the called utilities (like `wait_for`).
## Individual Comments
### Comment 1
<location path="src/widgetastic/browser.py" line_range="102" />
<code_context>
return create_widget_logger(type(self).__name__, self.browser.logger)
- def ensure_page_safe(self, timeout: str = "10s") -> None:
+ def ensure_page_safe(self, timeout=10) -> None:
# THIS ONE SHOULD ALWAYS USE JAVASCRIPT ONLY, NO OTHER SELENIUM INTERACTION
</code_context>
<issue_to_address>
**suggestion:** Consider keeping a concrete type annotation for `timeout` now that it is numeric seconds.
Since the default is now a bare `10`, this parameter is effectively “seconds as a number”. A concrete annotation like `timeout: int | float = 10` would improve static checking and make it clear that string formats like "10s" are no longer accepted, which helps callers migrating to the new convention.
```suggestion
def ensure_page_safe(self, timeout: int | float = 10) -> None:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return create_widget_logger(type(self).__name__, self.browser.logger) | ||
|
|
||
| def ensure_page_safe(self, timeout: str = "10s") -> None: | ||
| def ensure_page_safe(self, timeout=10) -> None: |
There was a problem hiding this comment.
suggestion: Consider keeping a concrete type annotation for timeout now that it is numeric seconds.
Since the default is now a bare 10, this parameter is effectively “seconds as a number”. A concrete annotation like timeout: int | float = 10 would improve static checking and make it clear that string formats like "10s" are no longer accepted, which helps callers migrating to the new convention.
| def ensure_page_safe(self, timeout=10) -> None: | |
| def ensure_page_safe(self, timeout: int | float = 10) -> None: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## legacy-selenium-support #309 +/- ##
===========================================================
- Coverage 86.54% 86.35% -0.20%
===========================================================
Files 18 18
Lines 2572 2572
===========================================================
- Hits 2226 2221 -5
- Misses 346 351 +5
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:
|
e018e97
into
RedHatQE:legacy-selenium-support
This PR is CP of for
legacy-selenium-supportbranch that is used in https://github.com/SatelliteQE/airgun.From the parent PR:
The timeout kwarg should be an integer, float, or timedelta instance
Also, num_sec will be deprecated.
Summary by Sourcery
Standardize timeout handling on integer-based
timeoutparameters instead of string-based ornum_secarguments across browser safety checks, element lookup, and wait strategies.Enhancements:
num_secparameter to thetimeoutparameter for consistency with the underlying wait utility.