sadpandajoe commented on code in PR #44738:
URL: https://github.com/apache/superset/pull/44738#discussion_r4133839026
##########
.github/workflows/pre-commit.yml:
##########
@@ -95,9 +58,24 @@ jobs:
run: |
set -euo pipefail
+ emit_needs() {
+ # $1: mode (all/none/files-with-$files-set-in-scope)
Review Comment:
This comment describes `emit_needs()` as accepting a `files` mode, but the
`case` below only handles `all`/`none` — calling it with `"files"` would
silently fall through and emit neither `needs_frontend` nor `needs_docs`. Could
this drop `files` from the comment so it doesn't mislead a future caller into
assuming that mode is handled here?
##########
scripts/pre_commit_dependency_needs.py:
##########
@@ -0,0 +1,80 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Decides whether pre-commit.yml's frontend/docs dependency installs are
+needed for a given set of changed files.
+
+Every hook that actually needs those installs is already path-gated in
+.pre-commit-config.yaml (oxfmt-frontend, oxlint-frontend,
custom-rules-frontend,
+stylelint-frontend, type-checking-frontend on `superset-frontend/`; oxlint-docs
+on `docs/*.{js,jsx,ts,tsx}`), so a changed-file list that matches none of these
+patterns means npm ci / yarn install can be skipped without any hook missing
+its dependencies.
+
+A previous inline-bash version of this (`printf '%s\\n' "$files" | grep -q
+...`) risked a false "not needed" result: under `set -o pipefail`, grep -q's
+early exit on a match can SIGPIPE a producer still mid-write for a large
+enough list, and that failure surfaces as the `if` condition's own status.
+Plain Python reading stdin to EOF has no equivalent early-exit-close
+behavior, so it isn't exposed to that failure mode at all.
+"""
+
+import os
+import re
+import sys
+from typing import List
+
+# Mirrors oxfmt-frontend/oxlint-frontend/custom-rules-frontend/
+# stylelint-frontend/type-checking-frontend's own `files:` prefix.
+FRONTEND_PATTERNS: List[str] = [r"^superset-frontend/"]
Review Comment:
This pattern matches every path under `superset-frontend/`, not just the
file types the frontend hooks
(`oxfmt-frontend`/`oxlint-frontend`/`custom-rules-frontend`/`stylelint-frontend`/`type-checking-frontend`)
actually lint. So a PR touching only `superset-frontend/README.md`, or any
other file under that tree outside `.{js,jsx,ts,tsx,css,scss,sass,json}`, still
runs the full `npm ci` even though none of those hooks fire on it — leaving
some of the intended savings on the table for a common case. The patterns here
and in `.pre-commit-config.yaml` also have nothing keeping them in sync going
forward. Could this narrow the pattern to the hooks' actual extensions?
##########
tests/unit_tests/scripts/pre_commit_dependency_needs_test.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import subprocess
+import sys
+from pathlib import Path
+
+from scripts import pre_commit_dependency_needs as needs
+
+REPO_ROOT = Path(__file__).resolve().parents[3]
+SCRIPT_PATH = REPO_ROOT / "scripts" / "pre_commit_dependency_needs.py"
+
+
+def test_pure_backend_change_needs_neither() -> None:
+ files = ["superset/commands/database/update.py", "tests/unit_tests/foo.py"]
+ assert needs.needs_frontend(files) is False
+ assert needs.needs_docs(files) is False
+
+
+def test_pure_frontend_change_needs_frontend_only() -> None:
+ files = ["superset-frontend/src/preamble.ts"]
+ assert needs.needs_frontend(files) is True
+ assert needs.needs_docs(files) is False
+
+
+def test_mixed_backend_and_frontend_change_needs_frontend() -> None:
+ files = ["superset/foo.py", "superset-frontend/src/bar.ts"]
+ assert needs.needs_frontend(files) is True
+ assert needs.needs_docs(files) is False
+
+
+def test_docs_prose_change_needs_neither() -> None:
+ """oxlint-docs only lints JS/TS under docs/, so a prose-only docs PR
+ (the common case) must not pay for the yarn install."""
+ files = ["docs/docs/some-page.mdx", "docs/docs/another.md"]
+ assert needs.needs_frontend(files) is False
+ assert needs.needs_docs(files) is False
+
+
+def test_docs_js_change_needs_docs_only() -> None:
+ files = ["docs/src/components/Foo.tsx"]
+ assert needs.needs_frontend(files) is False
+ assert needs.needs_docs(files) is True
+
+
+def test_empty_file_list_needs_neither() -> None:
+ assert needs.needs_frontend([]) is False
+ assert needs.needs_docs([]) is False
+
+
+def test_similarly_named_path_outside_the_real_directory_does_not_match() ->
None:
+ """A path that merely contains "superset-frontend" or "docs" as a
+ substring, without it being the actual path prefix, must not match --
+ _matches_any anchors with re.match, not a bare substring search."""
+ files = ["scripts/not-superset-frontend-related.py",
"some/docs-ish/file.py"]
+ assert needs.needs_frontend(files) is False
+ assert needs.needs_docs(files) is False
+
+
+def test_large_file_list_with_an_early_frontend_match() -> None:
+ """Regression test for the bug a previous `printf | grep -q` bash
+ implementation had: under `pipefail`, grep -q's early exit on a match
+ could SIGPIPE a producer still mid-write for a large list, corrupting
+ the result. A large list (order of magnitude past a typical 64KB pipe
+ buffer if this were piped) with the real match on the first line must
+ still resolve correctly -- plain Python has no equivalent failure mode,
+ which this pins."""
+ files = ["superset-frontend/early-match.ts"] + [
+ f"path/to/file_{i}.py" for i in range(5000)
+ ]
+ assert needs.needs_frontend(files) is True
+
+
+def test_main_writes_expected_outputs_to_github_output(tmp_path: Path) -> None:
Review Comment:
The only subprocess-level test here exercises just the frontend-positive
path (`needs_frontend=true`, `needs_docs=false`) — hardcoding
`needs_docs=false` in `main()` would still pass every test in this file. Could
this add a docs-positive case that asserts `needs_docs=true` through the actual
`main()`/`$GITHUB_OUTPUT` interface?
--
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]
