codeant-ai-for-open-source[bot] commented on PR #36896:
URL: https://github.com/apache/superset/pull/36896#issuecomment-3793822415
Question: - fixed patch.dict usage in
tests/integration_tests/thumbnails_tests.py to use the actual app.config object.
- improved robustness in superset-frontend/scripts/check-custom-rules.js by
removing process.exit(0) and handling missing dependencies (fast-glob)
gracefully.
- fixed python 3.8 compatibility in tests/common/logger_utils.py to allow
running tests locally.
please re-review.
Answer:
Thanks — I re-reviewed only the newly added/changed code in this PR (+
lines) and focused on the three areas you called out. Summary plus a few
targeted suggestions and potential edge-cases to consider.
High-level summary of relevant additions
- Frontend MixedTimeseries:
- Added a new stackGroup parameter to transformSeries and plumbing in
transformProps to enable grouped + stacked bars (first groupby dimension
becomes the stackGroup base; series in same stackGroup stack, different
stackGroups are side-by-side).
- transformProps also guards hooks with hooks || {}.
- Small adjustments to annotation style/emphasis handling.
- Added a unit test transformProps.test.ts that asserts series[].stack
values for a multi-groupby case.
- Tests:
- tests/integration_tests/thumbnails_tests.py: replaced
patch.dict("flask.current_app.config", ...) with patch.dict(app.config, ...)
and restructured context managers to avoid nested parentheses (improves test
robustness and clarity).
- scripts/check-custom-rules.js:
- Make @babel/parser and @babel/traverse optional (try/catch require).
- Replace glob with fast-glob usage, and handle missing fast-glob
gracefully (warn and return).
- Remove a hard process.exit(0) (as stated in PR description) and add
warnings when dependencies are missing.
- Python typing / compatibility:
- tests/common/logger_utils.py: adjusted typing (Type vs type[Any]) and
Decorated alias to support Python 3.8 typing forms — improves local test
compatibility.
Concrete feedback, risks, and suggested improvements
1) MixedTimeseries grouped + stacked logic
- What I see:
- transformProps passes stackGroup when groupby.length > 1. stackGroup is
taken from labelMap?.[seriesName]?.[0] or from entryName split on ', ' fallback
— then passed to transformSeries.
- transformSeries uses stackGroup as override for the computed stackId for
observation and trend series; stackIdSuffix is appended afterwards (e.g., '\na'
or '\nb').
- Suggestions / checks:
- labelMap access: labelMap?.[seriesName]?.[0] is used. Confirm labelMap
always contains the expected key for queries with multiple groupbys. If
labelMap is undefined or seriesName not present, you fall back to parsing
entryName, which is ok but brittle if label format changes.
- Suggest adding a small helper to consistently compute the
first-groupby-value (with tests for edge cases like single value, value
contains comma, missing labelMap).
- Entry name parsing: splitting by ', ' assumes delimiter and ordering. If
groupby values themselves can contain commas or whitespace, this could produce
incorrect stackGroup. Consider a safer separator or a helper that reads
label_map when available and only falls back to entryName when confident.
- Type coverage: transformSeries signature now includes stackGroup?:
string (comment added). If TypeScript types for SeriesOption consumers expect
certain fields for stack, ensure the added param is typed in any calling sites
(I see it used only here).
- Unit test: the added test is useful. Consider adding:
- a test where labelMap is absent and entryName includes commas (to
verify fallback).
- a test where groupby length === 1 (ensure no stackGroup is added).
- a test where multiple different first-groupby values produce different
stack IDs across queries (already covered).
- Suffix usage: tests expect stack 'SF\na' and 'SF\nb'. Ensure the newline
suffix convention ('\na', '\nb') is intentional and documented — it might be
non-obvious to contributors.
2) transformProps hooks guard
- Good change: const { setDataMask = () => {}, onContextMenu } = hooks ||
{}; prevents errors when hooks is undefined. That avoids runtime exceptions.
- Suggestion: if hooks is expected to be present in normal usage, consider
documenting whether hooks can be undefined (or prefer to default to empty
object earlier when constructing ChartProps).
3) tests/integration_tests/thumbnails_tests.py patch.dict usage
- Good fix: using patch.dict(app.config, ...) is more robust and correct
than patch.dict("flask.current_app.config", ...), especially for tests that use
different Flask contexts.
- Behavior change: you changed some compound with(...) blocks into
comma-separated context managers — functionally equivalent and cleaner.
4) check-custom-rules.js robustness
- Good points:
- Not hard-failing when @babel/parser or @babel/traverse not available,
and handling missing fast-glob by warning and returning, avoids crashing
pre-commit in environments that don't have those dev dependencies installed.
- Potential issue:
- You catch require for parser and traverse and only warn; later code
still uses parser/traverse. If either is undefined, later calls will throw. I
see you check for fast-glob before using it, but you do not early-return if
parser/traverse are missing. So:
- If parser or traverse failed to load, but fast-glob exists and
produces files, subsequent parsing/traversal will throw a runtime error.
- Recommendation: if parser or traverse is missing, either:
- early return (skip checks) after warning, or
- gate/guard later code paths to avoid using parser/traverse when
they’re undefined and produce a meaningful warning and non-zero exit code if
required by CI.
- Another note: you replaced glob.sync with fast-glob; if fast-glob isn't
present you return early. Good. Consider falling back to the built-in glob
module (which is in Node core) if fast-glob not present — that would keep
behavior similar but avoid the extra dependency.
5) scripts/check-custom-rules.sh and scripts/oxlint.sh chunking
- Good: processing file lists in chunks avoids command-line length issues on
Windows.
- Small suggestion: test chunking on Windows CI and ensure performance is
acceptable. Also consider making chunk-size a variable at top of script for
easier tuning.
6) tests/common/logger_utils.py Python typing changes
- Change to use Type from typing and use Decorated = Union[Type[Any],
Function] improves Python 3.8 compatibility — good.
- Minor nit: You imported Signature and signature from inspect but keep
empty_and_none = {Signature.empty, "None"} — ok.
7) superset/db_engine_specs/lint_metadata.py changes (visual / UX changes)
- You replaced emoji and block characters with text markers and changed
progress bars to ASCII # and -.
- Those changes make script output more portable (non-UTF8 terminals). Check
that internal tooling or docs do not rely on the previous emoji characters.
- You modified duplicate JoinedStr branch (two identical elifs) — I see you
added a repeated case: originally there was one JoinedStr, you added another
identical block. That duplicates code; probably unintentional. Code before had:
elif isinstance(node, ast.JoinedStr):
return "<f-string>"
You added another same branch — duplicates are harmless but confusing.
Suggest removing the duplicate branch.
8) Other small python CLI output tweaks
- You changed several messages to bracketed text like "[COMPLETE]" — OK;
just be mindful of any scripts parsing these outputs.
9) Tests refactors using chained context managers
- Many tests moved from multi-line with ( ... ) to chained "with a, b, c:"
forms (or backslash continuations). This is fine and reduces indentation/noise.
- Make sure no behavioral change occurred — I scanned and they appear
equivalent.
Actionable suggestions / follow-ups
- Add a unit test to cover the fallback path when label_map is absent and
entryName contains commas (to ensure stackGroup selection is stable).
- In check-custom-rules.js, either:
- return early if parser or traverse are missing (same as fast-glob flow),
or
- add guards for parser/traverse usage and produce a clear non-zero exit
if rule checks are required in CI.
Example: if (!parser || !traverse) { console.warn(...); return 0; }
- Remove the duplicated ast.JoinedStr branch in lint_metadata.py.
- Document the newline suffix convention for stackIdSuffix (why '\na'/'\nb'
etc.) in a comment or in code-level docs to avoid future regressions.
- Consider adding TypeScript typings or JSDoc for the new stackGroup
parameter to help maintainers in the frontend repo.
Overall impression
- The main feature (stackGroup) is implemented cleanly and has at least one
unit test covering the common multi-groupby scenario.
- The test and scripts robustness fixes are valuable.
- A couple of small follow-ups (guarding parser/traverse usage and removing
duplicate JoinedStr block) will make the changes safer/cleaner.
If you want, I can propose concrete code snippets for:
- an early-return guard in check-custom-rules.js if parser/traverse are
missing;
- a small helper to extract the first groupby value more robustly (with
examples).
--
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]
