aminghadersohi commented on code in PR #44254:
URL: https://github.com/apache/superset/pull/44254#discussion_r4020355651
##########
superset/utils/date_parser.py:
##########
@@ -492,17 +519,18 @@ def get_since_until( # pylint:
disable=too-many-arguments,too-many-locals,too-m
"""
separator = " : "
- _relative_start = relative_start if relative_start else "today"
_relative_end = relative_end if relative_end else "today"
if time_range == NO_TIME_RANGE or time_range == _(NO_TIME_RANGE):
return None, None
if time_range and time_range.startswith("Last") and separator not in
time_range:
- time_range = time_range + separator + _relative_end
+ _end = relative_end or get_default_bound_for_shorthand(time_range)
Review Comment:
bito's premise holds — `DEFAULT_RELATIVE_*_TIME="today"` is truthy, so this
`or` does skip the helper on the chart path — but its conclusion doesn't. I ran
that path at both refs: `Last hour`/`2 hours`/`5 hours` go ValueError → correct
range, fixed by the `hour` lookup change instead. Keep the `or`.
##########
superset/utils/date_parser.py:
##########
@@ -492,17 +519,18 @@ def get_since_until( # pylint:
disable=too-many-arguments,too-many-locals,too-m
"""
separator = " : "
- _relative_start = relative_start if relative_start else "today"
_relative_end = relative_end if relative_end else "today"
if time_range == NO_TIME_RANGE or time_range == _(NO_TIME_RANGE):
return None, None
if time_range and time_range.startswith("Last") and separator not in
time_range:
- time_range = time_range + separator + _relative_end
+ _end = relative_end or get_default_bound_for_shorthand(time_range)
+ time_range = time_range + separator + _end
if time_range and time_range.startswith("Next") and separator not in
time_range:
- time_range = _relative_start + separator + time_range
+ _start = relative_start or get_default_bound_for_shorthand(time_range)
Review Comment:
Newly load-bearing: the old default was the literal `"today"`, equal to
`DEFAULT_RELATIVE_START_TIME`, so dropping `relative_start or` was a no-op. Now
dropping it makes `Next hour` + `relative_start="today"` raise, and all 118
tests still pass. Fix belongs in the new test, not this span.
##########
tests/unit_tests/mcp_service/common/test_time_range_validation.py:
##########
@@ -103,10 +105,14 @@ def test_strips_surrounding_whitespace(self) -> None:
class TestValidateTimeRangeSubDayLast:
"""Sub-day ``Last ...`` values are rewritten to an explicit DATEADD range.
- get_since_until() pairs a sub-day since-expression (resolved against
- ``now``) with a default until of ``today`` (midnight), so since lands
- after until and it raises "From date cannot be larger than to date".
- Anchoring both ends on ``now`` fixes that.
+ get_since_until() itself now resolves a sub-day ``Last ...`` value
Review Comment:
Updated here but not in the module it describes:
`time_range_validation.py:80-84` still says `"Last Hour" fails exactly like
"Last hour"` (it now resolves) and that `Next <sub-day>` gets a midnight since
(now `now`); `_normalize_sub_day_last`'s docstring still describes the raise.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
