codeant-ai-for-open-source[bot] commented on PR #36896:
URL: https://github.com/apache/superset/pull/36896#issuecomment-3793863855
Question: - **resolved mapbox point radius scaling issue (#34398)**:
modified `transformprops.js` to ensure the `pointradius` prop is always a
number. previously, selecting a column would pass a string (column name), which
broke rendering and scaling logic.
- **fixed python 3.8 compatibility**: updated `tests/common/logger_utils.py`
to use `typing.type` instead of the built-in `type[]` syntax to resolve a
`typeerror` encountered in local test environments.
- **improved script robustness**: included previous fixes for
`check-custom-rules.js` to handle missing dependencies without crashing.
- **fixed backend test mocks**: corrected `patch.dict` usage in
`thumbnails_tests.py`.
please re-review.
Answer:
Thanks — I re-reviewed the new additions (+ lines) and the follow-up
questions. Short answers first, then targeted feedback and small, concrete
suggestions.
Summary answers (each bullet addresses the PR questions)
- resolved mapbox point radius scaling issue (#34398)
- Yes — the change in
superset-frontend/plugins/legacy-plugin-chart-map-box/src/transformProps.js
makes pointRadius numeric by defaulting to DEFAULT_POINT_RADIUS when the
incoming value is "Auto" or not a number:
- Added guard: pointRadius === 'Auto' || typeof pointRadius !== 'number'
? DEFAULT_POINT_RADIUS : pointRadius
- This prevents passing a string (e.g., a column name) into downstream map
rendering code and fixes the scaling/rendering bug.
- fixed python 3.8 compatibility
- Yes — tests/common/logger_utils.py now uses typing.Type and Type[Any] in
a few places and adjusts the Decorated alias. This avoids Python 3.8 errors
caused by using type[...] generics and should allow local tests to run on older
Python versions.
- improved script robustness (check-custom-rules.js)
- The script now:
- wraps requires for @babel/parser and @babel/traverse in try/catch and
logs a warning instead of crashing when missing.
- tries to use fast-glob and warns/returns early if fast-glob is missing.
- replaces glob.sync usage with fast-glob.
- removes abrupt process.exit(0) behavior.
- This makes the script friendlier in environments where dev dependencies
are not installed.
- fixed backend test mocks (thumbnails_tests.py)
- Yes — patch.dict now targets app.config, and the with(...) blocks were
simplified to chained context managers. This is the correct and more robust
approach for these tests.
Targeted feedback, risks, and small concrete suggestions (with examples)
1) MixedTimeseries: grouped + stacked (stackGroup)
- What I saw:
- transformProps sets a stackGroup when groupby.length > 1, taking
labelMap?.[seriesName]?.[0] or splitting entryName by ", " fallback.
transformSeries then uses stackGroup as the base stack id for observation/trend
series, and appends stackIdSuffix (e.g., '\na' or '\nb').
- A unit test was added to assert expected series[].stack values for a
city/gender example.
- Suggestions / risks:
- label_map fallback parsing (entryName.split(', ')[0]) is brittle if
values can include commas or different separators.
- Consider extracting the first groupby value using a small helper that:
- Uses label_map when available
- Falls back to a more robust parsing if necessary
- Handles cases where label_map is absent or malformed
- Also document the newline-based stackIdSuffix ('\na', '\nb') convention
so future contributors know why it's used.
- Example helper (TypeScript-like pseudocode to add in
transformProps/transformSeries):
- computeFirstGroupValue(seriesName, labelMap, entryName) {
if (labelMap?.[seriesName] && labelMap[seriesName].length > 0) {
return String(labelMap[seriesName][0]);
}
// fallback: try splitting by comma, but be tolerant
if (entryName.includes(', ')) return entryName.split(', ')[0];
// single token fallback
return entryName;
}
- Use this helper instead of repeating the logic inline. Add unit tests
for:
- missing label_map
- entryName containing commas in values
- groupby length == 1 (expect no stackGroup)
2) check-custom-rules.js — guard parser/traverse usage
- Issue: the script warns if @babel/parser or @babel/traverse are missing,
but later code still attempts to use parser/traverse if files are found. That
can cause a runtime crash.
- Recommendation: early-return if parser or traverse are not available (same
pattern used for fast-glob), or gracefully skip parsing steps. Example:
- after try/catch:
if (!parser || !traverse) {
console.warn('Skipping AST-based custom rules because @babel/parser or
@babel/traverse are missing.');
return;
}
- Alternatively fall back to using glob (Node core) if fast-glob is missing,
instead of returning — keeps functionality without the extra dependency.
3) superset/db_engine_specs/lint_metadata.py — duplicate branch and output
changes
- I noticed you have two identical elif isinstance(node, ast.JoinedStr)
branches; keep only one — duplication is confusing.
- You replaced emojis and block characters with ASCII; that's fine for
portability, but ensure no downstream tooling expects the previous characters.
- The progress-bar characters changed from block/░ to # and - — consistent
and portable.
4) Tests using patch.dict and with chaining
- The conversions to patch.dict(app.config, ...) and chained with statements
look correct and improve robustness — good.
5) scripts/check-custom-rules.sh and scripts/oxlint.sh chunking
- Chunking filenames is a good approach to avoid command-line length limits
on Windows.
- Consider exposing the chunk size as a variable near top of script for
easier tuning in CI:
- CHUNK_SIZE=100
- for ((i=0; i<${#js_ts_files[@]}; i+=CHUNK_SIZE)); do ...
- Please validate on Windows CI runners to ensure quoting and array-slicing
work as expected in the bash environment you target.
6) Small cleanups to consider
- Remove the duplicate ast.JoinedStr branch in lint_metadata.py.
- Add a unit test for the transformProps fallback path (label_map absent).
- Add a small comment documenting the stackIdSuffix newline convention.
- In check-custom-rules.js, prefer falling back to core 'glob' if fast-glob
missing (instead of returning), or do the early-return for parser/traverse as
noted above.
Overall impression
- The mapbox pointRadius fix and Python 3.8 typing fix are correct and
low-risk.
- The grouped+stacked bar support (stackGroup) implementation is sensible
and covered by a test; the main risk is the fallback parsing of entryName — a
robust helper + test coverage would mitigate this.
- The script robustness and tests/mocks fixes are helpful and make
local/developer environments more tolerant.
- Removing the duplicate AST branch and adding the guard in
check-custom-rules.js will make the patch cleaner and safer.
If you’d like, I can propose the exact small code snippet to add to
check-custom-rules.js (the early return guard) and a helper implementation for
first-group-by extraction with a unit test 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: [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]
