You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "kksingh000 (via GitHub)" <gi...@apache.org> on 2026/10/02 14:58:06 UTC
[PR] fix(change-detector): classify changed files by language, not directory [superset]
kksingh000 opened a new pull request, #44895:
URL: https://github.com/apache/superset/pull/44895
## What this changes
`scripts/change_detector.py`'s `PATTERNS` groups changed files by project area (`python` means "under `superset/`, `scripts/`, `tests/`, ..."; `frontend` means "under `superset-frontend/`"). That's the right signal for gating CI jobs by area, but it isn't a language classifier: a `.js` file living outside `superset-frontend/` -- e.g. `superset/mcp_service/index.js`, or a `.js` file under `scripts/` -- is grouped under `python` by `PATTERNS`, not `frontend`.
That gap was flagged during review of #44699, which wanted to reuse the `python`/`frontend` groups as a stand-in for "which CodeQL language(s) changed": https://github.com/apache/superset/pull/44699#discussion_r4133227541. Reusing a directory-based group as a language signal silently drops a JavaScript file that doesn't live under `superset-frontend/`. #44822 was opened to track it.
This PR adds a standalone, independent fix: `detect_languages()` classifies files by extension (`.py` -> `python`; `.js`/`.jsx`/`.ts`/`.tsx`/`.mjs`/`.cjs` -> `javascript`) regardless of directory, and `main()` now also writes a `languages` JSON-array output (e.g. `languages=["javascript", "python"]`) alongside the existing group outputs. The existing `PATTERNS` groups and their outputs (`python`, `frontend`, `docker`, `docs`, `superset-extensions-cli`) are untouched -- this is additive, so nothing that currently consumes those outputs changes behavior. The `languages` output is meant to be picked up by a future CodeQL `lang_matrix` step (e.g. once #44699 lands/is rebased), without this PR needing to touch that workflow itself.
Per [issue #44822](https://github.com/apache/superset/issues/44822#issuecomment-5926427475) and [hainenber's go-ahead](https://github.com/apache/superset/issues/44822#issuecomment-5954244193), this targets `master` independently of #44699. It does not touch the separate push-range bug also tracked under #44822, which #44833 already covers.
## Failure modes
- A `.py` file anywhere -> `languages` includes `python`; `PATTERNS["python"]` groups it the same as before (unchanged).
- A `.js`/`.ts`/`.jsx`/`.tsx` file under `superset-frontend/` -> `languages` includes `javascript`; `PATTERNS["frontend"]` groups it the same as before (unchanged).
- A `.js` file **outside** `superset-frontend/` (e.g. `superset/mcp_service/index.js`) -> `languages` now correctly includes `javascript`, even though `PATTERNS["python"]` still groups it under `python` (unchanged, since that's a separate directory-based signal still used for CI gating).
- A file with an unmapped extension (`.md`, `Dockerfile`, `.yml`, ...) -> contributes nothing to `languages`.
- `workflow_dispatch`/`schedule` runs (`files is None`) -> `languages` returns every known language, matching the existing "assume everything changed" behavior for the `PATTERNS` groups.
- A push/PR touching >= 99 files (the existing 100-file GitHub API cap workaround) -> `languages` also returns every known language, so a consumer combining both outputs never sees a language silently excluded by the cap.
## Verification
Added four new tests to `tests/unit_tests/scripts/change_detector_test.py` covering: the outside-`superset-frontend/` `.js` gap itself (and that it's still true `PATTERNS` doesn't fix it, which is why this is a separate signal), mixed Python+TypeScript changes, unmapped extensions, and the `None`/"assume everything changed" case. All existing tests in that file are unchanged.
Not run locally (no Python toolchain in my environment); the logic was sanity-checked standalone against the same inputs as the new tests before opening this PR. Please run CI -- happy to address any findings.
Fixes #44822 (the language-classification half; the push-range half is already covered by #44833, left untouched here).
--
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(change-detector): classify changed files by language, not directory [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44895:
URL: https://github.com/apache/superset/pull/44895#issuecomment-5956354381
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44895?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 65.96%. Comparing base ([`10d0685`](https://app.codecov.io/gh/apache/superset/commit/10d06859b834749ce2e48698d4ef77f998bc62f1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5ee996b`](https://app.codecov.io/gh/apache/superset/commit/5ee996bb5ce0127f2a7bb78d73f84c90cedbfe9c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (10d0685) and HEAD (5ee996b). Click for more details.
>
> <details><summary>HEAD has 3 uploads less than BASE</summary>
>
>| Flag | BASE (10d0685) | HEAD (5ee996b) |
>|------|------|------|
>|python|6|3|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44895 +/- ##
===========================================
- Coverage 82.08% 65.96% -16.12%
===========================================
Files 2995 2995
Lines 184988 184988
Branches 42814 42814
===========================================
- Hits 151848 122036 -29812
- Misses 30383 60270 +29887
+ Partials 2757 2682 -75
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44895/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/44895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.30% <ø> (?)` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.18% <ø> (ø)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/44895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.20% <ø> (-30.90%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.07% <ø> (ø)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44895/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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/44895?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(change-detector): classify changed files by language, not directory [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44895:
URL: https://github.com/apache/superset/pull/44895#issuecomment-5955768673
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 5ee996bb5ce0127f2a7bb78d73f84c90cedbfe9c |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6abfce949cb4f40008f8373e |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44895--superset-docs-preview.netlify.app](https://deploy-preview-44895--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><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#DYtBCoQwDAD_krOFgsXV_YHPSNNkLXTbUiso4t_NbWZgbqit_GuHL8AAvmGmTVniaWjD_GMTuDP10kxSPVALJdz3KJGwx5J1C1xTudag44ReiBe3kHfirLWzzONnZHhe) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44895--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(change-detector): classify changed files by language, not directory [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44895:
URL: https://github.com/apache/superset/pull/44895#issuecomment-5955139145
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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(change-detector): classify changed files by language, not directory [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 #44895:
URL: https://github.com/apache/superset/pull/44895#discussion_r4167235616
##########
scripts/change_detector.py:
##########
@@ -217,43 +254,10 @@ def main(event_type: str, sha: str, repo: str) -> None:
files, patterns_compiled
)
- # Output results
- output_path = os.getenv("GITHUB_OUTPUT") or "/tmp/GITHUB_OUTPUT.txt" # noqa: S108
- with open(output_path, "a") as f:
- for check, changed in changes_detected.items():
- # NOTE: as noted above, we assume that if 100 files are touched, we should
- # trigger all checks. This is a workaround for the GitHub API limit of 100
- # files. Using >= 99 because off-by-one errors are not uncommon
- if changed or (files is not None and len(files) >= 99):
- print(f"{check}=true", file=f)
- print(f"Triggering group: {check}")
-
-
-def get_git_sha() -> str:
- return os.getenv("GITHUB_SHA") or subprocess.check_output( # noqa: S603
- ["git", "rev-parse", "HEAD"] # noqa: S603, S607
- ).strip().decode("utf-8")
-
-
-if __name__ == "__main__":
- parser = argparse.ArgumentParser(
- description="Detect file changes based on event context"
- )
- parser.add_argument(
- "--event-type",
- default=os.getenv("GITHUB_EVENT_NAME") or "push",
- help="The type of event that triggered the workflow",
- )
- parser.add_argument(
- "--sha",
- default=get_git_sha(),
- help="The commit SHA for push events or PR head SHA",
- )
- parser.add_argument(
- "--repo",
- default=os.getenv("GITHUB_REPOSITORY") or "apache/superset",
- help="GitHub repository in the format owner/repo",
- )
- args = parser.parse_args()
-
- main(args.event_type, args.sha, args.repo)
+ # The 100-file API cap below treats a push/PR touching that many files as
+ # "everything changed" for the PATTERNS groups; language detection
+ # honors the same assumption so a consumer combining both outputs never
+ # sees a language silently excluded by the cap.
+ languages = (
+ sorted(set(LANGUAGE_EXTENSIONS.values()))
+ if files is not None and len(files)
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `5ee996b`.
The computed `languages` list is now written to the `GITHUB_OUTPUT` file as a JSON value with `print(f"languages={json.dumps(languages)}", file=f)`, exposing it to the composite action.
<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(change-detector): classify changed files by language, not directory [superset]
Posted by "kksingh000 (via GitHub)" <gi...@apache.org>.
kksingh000 commented on PR #44895:
URL: https://github.com/apache/superset/pull/44895#issuecomment-5955812362
Pushed a commit that reuses the `PATTERNS` group names rather than introducing a separate mapping:
```python
GROUP_LANGUAGE_EXTENSIONS: dict[str, dict[str, str]] = {
"python": {".py": "python"},
"frontend": {".js": "javascript", ".jsx": "javascript", ".ts": "javascript", ".tsx": "javascript", ".mjs": "javascript", ".cjs": "javascript"},
}
```
One scoping decision worth flagging: I did not rename `PATTERNS["frontend"]` itself to `javascript`. That exact key is consumed directly by `.github/actions/change-detector/action.yml`'s own `frontend` output plus `superset-e2e.yml`, `superset-frontend.yml`, `codeql-analysis.yml`, `docker.yml`, and `superset-translations.yml` -- all gating CI jobs by directory/area ("anything under `superset-frontend/` changed"), not by language. Renaming the key would silently change behavior for all of those, well outside this PR's scope, so `PATTERNS["frontend"]` and its existing consumers are untouched. Only the new language-detection code (`GROUP_LANGUAGE_EXTENSIONS` / `detect_languages`) adopts the javascript-oriented naming you asked for. Happy to do the full rename as its own reviewed PR if you'd still like `frontend` renamed everywhere -- just didn't want to bundle a wider, harder-to-review change into this one without checking first.
While implementing this I also ran into two bugs beyond what the bots flagged: the file as previously committed had a genuine unclosed parenthesis partway through `main()`, so it didn't parse at all (`ast.parse`/`py_compile` fails on it) -- the entire GITHUB_OUTPUT write loop, `get_git_sha()`, and the `__main__` entrypoint were missing and are now restored. Separately, `action.yml` had no `languages` entry in its `outputs:` block, so even a correctly emitted step output wouldn't have propagated out of the composite action to callers -- added it, which is also what CodeAnt's bot flagged and has now auto-verified as resolved against `5ee996b`.
Verified locally this time: the full `change_detector_test.py` suite (17/17, including 3 new tests covering the PATTERNS-name reuse, the 99-file cap, and that `main()` actually writes `languages` to GITHUB_OUTPUT), `ruff check` clean on both touched `.py` files, and `action.yml` validated as proper YAML.
--
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(change-detector): classify changed files by language, not directory [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber merged PR #44895:
URL: https://github.com/apache/superset/pull/44895
--
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(change-detector): classify changed files by language, not directory [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44895:
URL: https://github.com/apache/superset/pull/44895#issuecomment-5958889369
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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(change-detector): classify changed files by language, not directory [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 #44895:
URL: https://github.com/apache/superset/pull/44895#discussion_r4166973557
##########
scripts/change_detector.py:
##########
@@ -217,43 +254,10 @@ def main(event_type: str, sha: str, repo: str) -> None:
files, patterns_compiled
)
- # Output results
- output_path = os.getenv("GITHUB_OUTPUT") or "/tmp/GITHUB_OUTPUT.txt" # noqa: S108
- with open(output_path, "a") as f:
- for check, changed in changes_detected.items():
- # NOTE: as noted above, we assume that if 100 files are touched, we should
- # trigger all checks. This is a workaround for the GitHub API limit of 100
- # files. Using >= 99 because off-by-one errors are not uncommon
- if changed or (files is not None and len(files) >= 99):
- print(f"{check}=true", file=f)
- print(f"Triggering group: {check}")
-
-
-def get_git_sha() -> str:
- return os.getenv("GITHUB_SHA") or subprocess.check_output( # noqa: S603
- ["git", "rev-parse", "HEAD"] # noqa: S603, S607
- ).strip().decode("utf-8")
-
-
-if __name__ == "__main__":
- parser = argparse.ArgumentParser(
- description="Detect file changes based on event context"
- )
- parser.add_argument(
- "--event-type",
- default=os.getenv("GITHUB_EVENT_NAME") or "push",
- help="The type of event that triggered the workflow",
- )
- parser.add_argument(
- "--sha",
- default=get_git_sha(),
- help="The commit SHA for push events or PR head SHA",
- )
- parser.add_argument(
- "--repo",
- default=os.getenv("GITHUB_REPOSITORY") or "apache/superset",
- help="GitHub repository in the format owner/repo",
- )
- args = parser.parse_args()
-
- main(args.event_type, args.sha, args.repo)
+ # The 100-file API cap below treats a push/PR touching that many files as
+ # "everything changed" for the PATTERNS groups; language detection
+ # honors the same assumption so a consumer combining both outputs never
+ # sees a language silently excluded by the cap.
+ languages = (
+ sorted(set(LANGUAGE_EXTENSIONS.values()))
+ if files is not None and len(files)
Review Comment:
**Suggestion:** `languages` is assigned but never written to `GITHUB_OUTPUT`, so the composite action exposes no language result and CodeQL cannot use this classification.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Often` · 🏷️ `Incomplete implementation`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=add27a56c84f4e71bcdd13523ecad11e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=add27a56c84f4e71bcdd13523ecad11e&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:** scripts/change_detector.py
**Line:** 261:263
**Comment:**
*Incomplete Implementation: `languages` is assigned but never written to `GITHUB_OUTPUT`, so the composite action exposes no language result and CodeQL cannot use this classification.
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%2F44895&comment_hash=ea52bd5639715752e8e796e5bc1b4a9057400da42cd0b2c33eff30b669d51558&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44895&comment_hash=ea52bd5639715752e8e796e5bc1b4a9057400da42cd0b2c33eff30b669d51558&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(change-detector): classify changed files by language, not directory [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44895:
URL: https://github.com/apache/superset/pull/44895#issuecomment-5955235268
<!-- Bito Reply -->
The flagged issue is correct. The `languages` variable is calculated but not written to the `GITHUB_OUTPUT` file, making it unavailable to subsequent workflow steps. To resolve this, you should append the detected languages to the output file, similar to how other changes are handled.
Would you like me to implement this fix and check the remaining comments on this PR?
**scripts/change_detector.py**
```
output_path = os.getenv("GITHUB_OUTPUT") or "/tmp/GITHUB_OUTPUT.txt"
with open(output_path, "a") as f:
# ... existing logic ...
languages = detect_languages(files)
print(f"languages={','.join(languages)}", file=f)
```
--
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(change-detector): classify changed files by language, not directory [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44895:
URL: https://github.com/apache/superset/pull/44895#discussion_r4167113166
##########
scripts/change_detector.py:
##########
@@ -217,43 +254,10 @@ def main(event_type: str, sha: str, repo: str) -> None:
files, patterns_compiled
)
- # Output results
- output_path = os.getenv("GITHUB_OUTPUT") or "/tmp/GITHUB_OUTPUT.txt" # noqa: S108
- with open(output_path, "a") as f:
- for check, changed in changes_detected.items():
- # NOTE: as noted above, we assume that if 100 files are touched, we should
- # trigger all checks. This is a workaround for the GitHub API limit of 100
- # files. Using >= 99 because off-by-one errors are not uncommon
- if changed or (files is not None and len(files) >= 99):
- print(f"{check}=true", file=f)
- print(f"Triggering group: {check}")
-
-
-def get_git_sha() -> str:
- return os.getenv("GITHUB_SHA") or subprocess.check_output( # noqa: S603
- ["git", "rev-parse", "HEAD"] # noqa: S603, S607
- ).strip().decode("utf-8")
-
-
-if __name__ == "__main__":
- parser = argparse.ArgumentParser(
- description="Detect file changes based on event context"
- )
- parser.add_argument(
- "--event-type",
- default=os.getenv("GITHUB_EVENT_NAME") or "push",
- help="The type of event that triggered the workflow",
- )
- parser.add_argument(
- "--sha",
- default=get_git_sha(),
- help="The commit SHA for push events or PR head SHA",
- )
- parser.add_argument(
- "--repo",
- default=os.getenv("GITHUB_REPOSITORY") or "apache/superset",
- help="GitHub repository in the format owner/repo",
- )
- args = parser.parse_args()
-
- main(args.event_type, args.sha, args.repo)
+ # The 100-file API cap below treats a push/PR touching that many files as
+ # "everything changed" for the PATTERNS groups; language detection
+ # honors the same assumption so a consumer combining both outputs never
+ # sees a language silently excluded by the cap.
+ languages = (
+ sorted(set(LANGUAGE_EXTENSIONS.values()))
+ if files is not None and len(files)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unclosed expression SyntaxError</b></div>
<div id="fix">
The `languages = (` expression at line 261 is never closed — the file ends at line 263 mid-ternary, so `python -m py_compile` fails with `SyntaxError: '(' was never closed`. The module cannot be imported, which also breaks `tests/unit_tests/scripts/change_detector_test.py` (`from scripts import change_detector`). Complete the expression with the `>= 99` cap and an `else` branch.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
languages = (
sorted(set(LANGUAGE_EXTENSIONS.values()))
if files is not None and len(files) >= 99
else detect_languages(files)
)
````
</div>
</details>
</div>
<small><i>Code Review Run #d63e48</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
scripts/change_detector.py:
##########
@@ -217,43 +254,10 @@ def main(event_type: str, sha: str, repo: str) -> None:
files, patterns_compiled
)
- # Output results
- output_path = os.getenv("GITHUB_OUTPUT") or "/tmp/GITHUB_OUTPUT.txt" # noqa: S108
- with open(output_path, "a") as f:
- for check, changed in changes_detected.items():
- # NOTE: as noted above, we assume that if 100 files are touched, we should
- # trigger all checks. This is a workaround for the GitHub API limit of 100
- # files. Using >= 99 because off-by-one errors are not uncommon
- if changed or (files is not None and len(files) >= 99):
- print(f"{check}=true", file=f)
- print(f"Triggering group: {check}")
-
-
-def get_git_sha() -> str:
- return os.getenv("GITHUB_SHA") or subprocess.check_output( # noqa: S603
- ["git", "rev-parse", "HEAD"] # noqa: S603, S607
- ).strip().decode("utf-8")
-
-
-if __name__ == "__main__":
- parser = argparse.ArgumentParser(
- description="Detect file changes based on event context"
- )
- parser.add_argument(
- "--event-type",
- default=os.getenv("GITHUB_EVENT_NAME") or "push",
- help="The type of event that triggered the workflow",
- )
- parser.add_argument(
- "--sha",
- default=get_git_sha(),
- help="The commit SHA for push events or PR head SHA",
- )
- parser.add_argument(
- "--repo",
- default=os.getenv("GITHUB_REPOSITORY") or "apache/superset",
- help="GitHub repository in the format owner/repo",
- )
- args = parser.parse_args()
-
- main(args.event_type, args.sha, args.repo)
+ # The 100-file API cap below treats a push/PR touching that many files as
+ # "everything changed" for the PATTERNS groups; language detection
+ # honors the same assumption so a consumer combining both outputs never
+ # sees a language silently excluded by the cap.
+ languages = (
+ sorted(set(LANGUAGE_EXTENSIONS.values()))
+ if files is not None and len(files)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead computed value</b></div>
<div id="fix">
Even after the syntax is fixed, `languages` is computed at lines 261-263 but never read — no print, no GITHUB_OUTPUT write, no return (grep shows the only reference is the assignment). The diff removed the output block that wrote `{check}=true`, so the language-detection feature produces no output for any consumer.
</div>
</div>
<small><i>Code Review Run #d63e48</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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