You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org> on 2026/10/14 06:03:24 UTC

[PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

SEPURI-SAI-KRISHNA opened a new pull request, #45260:
URL: https://github.com/apache/superset/pull/45260

   ### SUMMARY
   
   Fixes #45259.
   
   Two `HOLIDAY()` time ranges returned a 500. Both now return what they should.
   
   **Giving the country without the datetime.** Both arguments after the holiday
   name are optional in the grammar, so `holiday("Christmas Day", "UK")` parses.
   `eval` read the second argument as the datetime whatever it held, so it took
   `.year` off a string and the request died with
   `'str' object has no attribute 'year'`.
   
   The two kinds of argument are distinguishable: a datetime is always a function
   call (`datetime(...)`, `dateadd(...)`, ...) and a country is always quoted text.
   Resolving the optional arguments by kind rather than by position makes the
   two-argument form work instead of crashing, and leaves the three documented
   forms unchanged.
   
   **A country with no calendar.** `country_holidays` raises `NotImplementedError`
   for a country it has no calendar for. That is the caller's to correct, so it now
   arrives as a `ValueError` naming the country, which the
   `/api/v1/time_range/` handler already maps to a 400, the same way an unknown
   holiday does. The accepted spellings are the ISO code and the package's
   registered name, so `"us"` (right code, wrong case) and `"United Kingdom"`
   (written out in full) both used to 500.
   
   Case is not normalised: registered names such as `"Germany"` are not uppercase,
   so upper-casing the argument would break inputs that work today. The error
   message is the fix for those.
   
   The in-UI syntax help gains the two-argument form it now supports.
   
   I swept 271 grammar-valid expressions across `DATETIME`, `DATEADD`, `DATEDIFF`,
   `DATETRUNC`, `LASTDAY` and `HOLIDAY`. These two were the only ones raising
   anything other than `ValueError`, and after the change the sweep raises nothing
   but `ValueError`.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   
   Before:
   
   ```
   GET /api/v1/time_range/?q='HOLIDAY("Boxing Day", "UK") : DATETIME("2027-01-01")'
   500 {"error": "'str' object has no attribute 'year'"}
   
   GET /api/v1/time_range/?q='HOLIDAY("Christmas Day", DATETIME("2019"), "us") : now'
   500 {"error": "Country us not available"}
   ```
   
   After:
   
   ```
   GET /api/v1/time_range/?q='HOLIDAY("Boxing Day", "UK") : DATETIME("2027-01-01")'
   200 {"result": [{"since": "2026-12-26T00:00:00", "until": "2027-01-01T00:00:00", ...}]}
   
   GET /api/v1/time_range/?q='HOLIDAY("Christmas Day", DATETIME("2019"), "us") : now'
   400 {"message": "Unexpected time range: Unable to find such a country: [us]"}
   ```
   
   ### TESTING INSTRUCTIONS
   
   Explore any chart with a temporal column, open the Time Range control, switch to
   Advanced, and set START to `HOLIDAY("Boxing Day", "UK")` and END to
   `DATETIME("2027-01-01")`. The preview resolves to 2026-12-26 rather than
   failing. Setting a country such as `United Kingdom` reports that no such country
   was found instead of failing.
   
   New unit tests:
   
   ```
   pytest tests/unit_tests/utils/date_parser_tests.py -k holiday
   ```
   
   `test_holiday_reads_a_country_given_without_a_datetime` uses Boxing Day, which
   the UK calendar has and the default US calendar does not, so it fails if the
   country argument is read but ignored as well as if it crashes.
   
   Full file: `pytest tests/unit_tests/utils/date_parser_tests.py` passes.
   
   ### ADDITIONAL INFORMATION
   
   - [x] Has associated issue: #45259
   - [ ] Required feature flags:
   - [x] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   The UI change is two lines added to the time-range function help: the
   `holiday([holiday string], [country name])` syntax line and a matching example.
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45260:
URL: https://github.com/apache/superset/pull/45260#discussion_r4281081782


##########
superset-frontend/src/explore/components/controls/DateFilterControl/components/DateFunctionTooltip.tsx:
##########
@@ -90,6 +90,7 @@ dateunit = (year | month | week)`}</code>
       <pre>
         <code>{`holiday([string])
 holiday([holiday string], [datetime])
+holiday([holiday string], [country name])

Review Comment:
   ✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `8bfec31`.
   
   The holiday syntax now labels the argument as `[country code]`, and the examples use country codes such as `UK`.
   
   <sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
   
   <!-- codeant-auto-resolve-reply -->



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "SEPURI-SAI-KRISHNA (via GitHub) via notifications" <no...@superset.apache.org>.
SEPURI-SAI-KRISHNA commented on PR #45260:
URL: https://github.com/apache/superset/pull/45260#issuecomment-6607534080

   `lint-frontend` here is red for something outside this diff, so this one needs a
   re-run from someone who can trigger it.
   
   The errors are TS6305 and TS2305 in `plugin-chart-ag-grid-table` and
   `superset-ui-chart-controls`, from `superset-ui-chart-controls/lib/index.d.ts`
   not being built before the plugin is type-checked. The single frontend file in
   this PR, `DateFunctionTooltip.tsx`, appears nowhere in the failure log, and
   `lint-frontend` is green on other frontend PRs in the same window (#45935,
   #45913).
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on PR #45260:
URL: https://github.com/apache/superset/pull/45260#issuecomment-6162669104

   Good catch. The syntax block already said `[country name]` for the
   three-argument form before this change, and I repeated that wording on the
   two-argument form.
   
   `country_holidays` accepts an ISO code or the holidays package's own registered
   name, so `UK`, `GB`, `Germany` and `Canada` resolve while `United Kingdom` and
   `us` do not. The code is the form that works for every supported country, so
   both syntax lines now say `[country code]`.
   
   Accepting full names would need a name-to-code mapping, which is a separate
   change. What this PR fixes is that an unsupported value returns a 400 naming the
   country instead of a 500.
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45260:
URL: https://github.com/apache/superset/pull/45260#discussion_r4278475756


##########
superset-frontend/src/explore/components/controls/DateFilterControl/components/DateFunctionTooltip.tsx:
##########
@@ -90,6 +90,7 @@ dateunit = (year | month | week)`}</code>
       <pre>
         <code>{`holiday([string])
 holiday([holiday string], [datetime])
+holiday([holiday string], [country name])

Review Comment:
   ✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `10d5b19`.
   
   The holiday syntax now labels the argument as `[country code]`, and the examples use country codes such as `UK`.
   
   <sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
   
   <!-- codeant-auto-resolve-reply -->



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "EnxDev (via GitHub) via notifications" <no...@superset.apache.org>.
EnxDev merged PR #45260:
URL: https://github.com/apache/superset/pull/45260


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on code in PR #45260:
URL: https://github.com/apache/superset/pull/45260#discussion_r4278475335


##########
superset/utils/date_parser.py:
##########
@@ -905,15 +905,34 @@ def __init__(self, tokens: ParseResults) -> None:
 
     def eval(self) -> datetime:
         holiday = self.value[0].eval()
-        dttm, country = [None, None]
-        if len(self.value) >= 2:
-            dttm = self.value[1].eval()
-        if len(self.value) == 3:
-            country = self.value[2]
+        dttm: datetime | None = None
+        country: str | None = None
+        # Both arguments after the holiday name are optional, so the second one
+        # is the datetime or the country depending on which were supplied. What
+        # tells them apart is the kind of argument: a datetime is always a
+        # function call (`datetime(...)`, `dateadd(...)`, ...) and a country is
+        # always quoted text. Reading the second argument as the datetime
+        # whatever it held left `holiday("Christmas Day", "UK")` taking `.year`
+        # off a string.

Review Comment:
   Taken, that reads better. Dropped the last sentence so the comment only describes what the code does:
   
   ```python
   # Both arguments after the holiday name are optional, so the second one
   # is the datetime or the country depending on which were supplied. What
   # tells them apart is the kind of argument: a datetime is always a
   # function call (`datetime(...)`, `dateadd(...)`, ...) and a country is
   # always quoted text.
   ```
   
   Also merged master in, which should clear the `check-openapi-spec-drift` red. You were right that it was unrelated: that run is from before #45262 landed, and the check is green on master now.
   



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45260:
URL: https://github.com/apache/superset/pull/45260#issuecomment-6162341574

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45260?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `9.09091%` with `10 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 64.68%. Comparing base ([`e9862fb`](https://app.codecov.io/gh/apache/superset/commit/e9862fb4ab2f5f64ff39d3327a4d939c0519e145?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b1ff49b`](https://app.codecov.io/gh/apache/superset/commit/b1ff49b2151cba382fdcb41c848e6d7c7f6c215f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 133 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/45260?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [superset/utils/date\_parser.py](https://app.codecov.io/gh/apache/superset/pull/45260?src=pr&el=tree&filepath=superset%2Futils%2Fdate_parser.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvZGF0ZV9wYXJzZXIucHk=) | 9.09% | [10 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/45260?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #45260      +/-   ##
   ==========================================
   + Coverage   57.40%   64.68%   +7.27%     
   ==========================================
     Files        3013     3034      +21     
     Lines      188657   197796    +9139     
     Branches    43865    45947    +2082     
   ==========================================
   + Hits       108307   127953   +19646     
   + Misses      79301    66933   -12368     
   - Partials     1049     2910    +1861     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/45260/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/45260/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.97% <9.09%> (-1.18%)` | :arrow_down: |
   | [javascript](https://app.codecov.io/gh/apache/superset/pull/45260/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.53% <ø> (-0.01%)` | :arrow_down: |
   | [postgres](https://app.codecov.io/gh/apache/superset/pull/45260/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.99% <9.09%> (?)` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/45260/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.74% <9.09%> (-1.27%)` | :arrow_down: |
   | [python](https://app.codecov.io/gh/apache/superset/pull/45260/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.07% <9.09%> (+15.02%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45260?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on code in PR #45260:
URL: https://github.com/apache/superset/pull/45260#discussion_r4278473913


##########
superset/utils/date_parser.py:
##########
@@ -905,15 +905,34 @@ def __init__(self, tokens: ParseResults) -> None:
 
     def eval(self) -> datetime:
         holiday = self.value[0].eval()
-        dttm, country = [None, None]
-        if len(self.value) >= 2:
-            dttm = self.value[1].eval()
-        if len(self.value) == 3:
-            country = self.value[2]
+        dttm: datetime | None = None
+        country: str | None = None
+        # Both arguments after the holiday name are optional, so the second one
+        # is the datetime or the country depending on which were supplied. What
+        # tells them apart is the kind of argument: a datetime is always a
+        # function call (`datetime(...)`, `dateadd(...)`, ...) and a country is
+        # always quoted text. Reading the second argument as the datetime
+        # whatever it held left `holiday("Christmas Day", "UK")` taking `.year`
+        # off a string.

Review Comment:
   Taken, that reads better. Dropped the last sentence so the comment only describes what the code does:
   
   ```python
   # Both arguments after the holiday name are optional, so the second one
   # is the datetime or the country depending on which were supplied. What
   # tells them apart is the kind of argument: a datetime is always a
   # function call (`datetime(...)`, `dateadd(...)`, ...) and a country is
   # always quoted text.
   ```
   
   Also merged master in, which should clear the `check-openapi-spec-drift` red. You were right that it was unrelated: that run is from before #45262 landed, and the check is green on master now.
   



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45260:
URL: https://github.com/apache/superset/pull/45260#issuecomment-6162124943

   ## CodeAnt PR Risk: Medium Risk
   
   - The PR needs attention before merging because its tooltip advertises country names that the parser rejects.
   - The new test explicitly rejects “United Kingdom”; document supported country codes or accept full country names.
   
   **Assessed commit:** `b1ff49b2151c`


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #45260:
URL: https://github.com/apache/superset/pull/45260#discussion_r4276222747


##########
superset/utils/date_parser.py:
##########
@@ -905,15 +905,34 @@ def __init__(self, tokens: ParseResults) -> None:
 
     def eval(self) -> datetime:
         holiday = self.value[0].eval()
-        dttm, country = [None, None]
-        if len(self.value) >= 2:
-            dttm = self.value[1].eval()
-        if len(self.value) == 3:
-            country = self.value[2]
+        dttm: datetime | None = None
+        country: str | None = None
+        # Both arguments after the holiday name are optional, so the second one
+        # is the datetime or the country depending on which were supplied. What
+        # tells them apart is the kind of argument: a datetime is always a
+        # function call (`datetime(...)`, `dateadd(...)`, ...) and a country is
+        # always quoted text. Reading the second argument as the datetime
+        # whatever it held left `holiday("Christmas Day", "UK")` taking `.year`
+        # off a string.

Review Comment:
   Nit, take it or leave it. The last sentence describes the old bug rather than the code, so it reads oddly once this is merged; the PR description already covers it.
   
   ```suggestion
           # always quoted text.
   ```



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.
SEPURI-SAI-KRISHNA commented on code in PR #45260:
URL: https://github.com/apache/superset/pull/45260#discussion_r4278475335


##########
superset/utils/date_parser.py:
##########
@@ -905,15 +905,34 @@ def __init__(self, tokens: ParseResults) -> None:
 
     def eval(self) -> datetime:
         holiday = self.value[0].eval()
-        dttm, country = [None, None]
-        if len(self.value) >= 2:
-            dttm = self.value[1].eval()
-        if len(self.value) == 3:
-            country = self.value[2]
+        dttm: datetime | None = None
+        country: str | None = None
+        # Both arguments after the holiday name are optional, so the second one
+        # is the datetime or the country depending on which were supplied. What
+        # tells them apart is the kind of argument: a datetime is always a
+        # function call (`datetime(...)`, `dateadd(...)`, ...) and a country is
+        # always quoted text. Reading the second argument as the datetime
+        # whatever it held left `holiday("Christmas Day", "UK")` taking `.year`
+        # off a string.

Review Comment:
   Taken, that reads better. Dropped the last sentence so the comment only describes what the code does:
   
   ```python
   # Both arguments after the holiday name are optional, so the second one
   # is the datetime or the country depending on which were supplied. What
   # tells them apart is the kind of argument: a datetime is always a
   # function call (`datetime(...)`, `dateadd(...)`, ...) and a country is
   # always quoted text.
   ```
   
   Also merged master in, which should clear the `check-openapi-spec-drift` red. You were right that it was unrelated: that run is from before #45262 landed, and the check is green on master now.
   



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45260:
URL: https://github.com/apache/superset/pull/45260#discussion_r4408806646


##########
superset-frontend/src/explore/components/controls/DateFilterControl/components/DateFunctionTooltip.tsx:
##########
@@ -90,6 +90,7 @@ dateunit = (year | month | week)`}</code>
       <pre>
         <code>{`holiday([string])
 holiday([holiday string], [datetime])
+holiday([holiday string], [country name])

Review Comment:
   ✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `fb4b4a0`.
   
   The syntax now labels the argument as `[country code]` instead of implying it accepts a country name; the examples use `"UK"`.
   
   <sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
   
   <!-- codeant-auto-resolve-reply -->



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45260:
URL: https://github.com/apache/superset/pull/45260#discussion_r4278801164


##########
superset-frontend/src/explore/components/controls/DateFilterControl/components/DateFunctionTooltip.tsx:
##########
@@ -90,6 +90,7 @@ dateunit = (year | month | week)`}</code>
       <pre>
         <code>{`holiday([string])
 holiday([holiday string], [datetime])
+holiday([holiday string], [country name])

Review Comment:
   ✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `95866b4`.
   
   The holiday syntax now labels the argument as `[country code]`, and the examples use country codes such as `UK`.
   
   <sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
   
   <!-- codeant-auto-resolve-reply -->



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45260:
URL: https://github.com/apache/superset/pull/45260#issuecomment-6162058197

   ### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
   
   
   |  Name | Link |
   |:-:|------------------------|
   |<span aria-hidden="true">🔨</span> Latest commit | b1ff49b2151cba382fdcb41c848e6d7c7f6c215f |
   |<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6acf1b2da2996aa7c01f512e |
   |<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-45260--superset-docs-preview.netlify.app](https://deploy-preview-45260--superset-docs-preview.netlify.app) |
   |<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br />![QR Code](https://app.netlify.com/qr-code/eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTQ1MjYwLS1zdXBlcnNldC1kb2NzLXByZXZpZXcubmV0bGlmeS5hcHAifQ.e-lp1C6gXm3UnJN3OqZeulO28JcOxnzpmmNw7w7z5bs)<br /><br />_Use your smartphone camera to open QR code link._</details> |
   |<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpBCoAgEEDRu8w6KYWKukHHGHVMwVImF0V092b34b8XKpejNlgBOrCMp4vSId19LDl5fFRLBykZOyliLnwJ9FRzeTYvdEIXtDUezbJMiLMbdBi1Ifh-) |
   ---
   <!-- [superset-docs-preview Preview](https://deploy-preview-45260--superset-docs-preview.netlify.app) -->
   _To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45260:
URL: https://github.com/apache/superset/pull/45260#discussion_r4267647062


##########
superset-frontend/src/explore/components/controls/DateFilterControl/components/DateFunctionTooltip.tsx:
##########
@@ -90,6 +90,7 @@ dateunit = (year | month | week)`}</code>
       <pre>
         <code>{`holiday([string])
 holiday([holiday string], [datetime])
+holiday([holiday string], [country name])

Review Comment:
   ✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `e8ae0ee`.
   
   The holiday syntax now labels the argument as `[country code]`, and the examples use country codes such as `UK`.
   
   <sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
   
   <!-- codeant-auto-resolve-reply -->



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45260:
URL: https://github.com/apache/superset/pull/45260#discussion_r4267368468


##########
superset-frontend/src/explore/components/controls/DateFilterControl/components/DateFunctionTooltip.tsx:
##########
@@ -90,6 +90,7 @@ dateunit = (year | month | week)`}</code>
       <pre>
         <code>{`holiday([string])
 holiday([holiday string], [datetime])
+holiday([holiday string], [country name])

Review Comment:
   **Suggestion:** The syntax promises a country name, but full names such as “United Kingdom” are rejected, so users following it receive a `ValueError`.
   
   **Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes` · 🏷️ `Api mismatch`
   
   [![Use CodeAnt Skill](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/use-codeant-skill-flat-v2.svg)](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [![Fix in Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=970a834c02bb43ed96757146d65a8111&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [![Fix in VSCode Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=970a834c02bb43ed96757146d65a8111&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset-frontend/src/explore/components/controls/DateFilterControl/components/DateFunctionTooltip.tsx
   **Line:** 93:93
   **Comment:**
   	*Api Mismatch: The syntax promises a country name, but full names such as “United Kingdom” are rejected, so users following it receive a `ValueError`.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
   Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45260&comment_hash=890f92bfb023da625fe9e236db1340128597de3f385a9d5f9bd353e4a61e5289&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45260&comment_hash=890f92bfb023da625fe9e236db1340128597de3f385a9d5f9bd353e4a61e5289&reaction=dislike'>👎</a>



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45260:
URL: https://github.com/apache/superset/pull/45260#discussion_r4415329332


##########
superset-frontend/src/explore/components/controls/DateFilterControl/components/DateFunctionTooltip.tsx:
##########
@@ -90,6 +90,7 @@ dateunit = (year | month | week)`}</code>
       <pre>
         <code>{`holiday([string])
 holiday([holiday string], [datetime])
+holiday([holiday string], [country name])

Review Comment:
   ✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `24a5308`.
   
   The syntax now labels the argument as `[country code]` instead of implying it accepts a country name; the examples use `"UK"`.
   
   <sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
   
   <!-- codeant-auto-resolve-reply -->



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(date-parser): stop HOLIDAY returning 500 for its country argument [superset]

Posted by "EnxDev (via GitHub) via notifications" <no...@superset.apache.org>.
EnxDev commented on PR #45260:
URL: https://github.com/apache/superset/pull/45260#issuecomment-6626900378

   > `lint-frontend` here is red for something outside this diff, so this one needs a re-run from someone who can trigger it.
   > 
   > The errors are TS6305 and TS2305 in `plugin-chart-ag-grid-table` and `superset-ui-chart-controls`, from `superset-ui-chart-controls/lib/index.d.ts` not being built before the plugin is type-checked. The single frontend file in this PR, `DateFunctionTooltip.tsx`, appears nowhere in the failure log, and `lint-frontend` is green on other frontend PRs in the same window (#45935, #45913).
   
   I reran the test.


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org