codeant-ai-for-open-source[bot] commented on PR #36983: URL: https://github.com/apache/superset/pull/36983#issuecomment-3724084548
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36983/files#diff-a3775152bef6d7a152e08f6612e95dec7672bad8d2247fe4e1eeee4996380412R421-R424'><strong>Behavior Change</strong></a><br>The new bounded whitespace quantifiers (\s{1,5} and \s{0,5}) intentionally limit accepted whitespace. This can break valid-but-unusual inputs that contain longer runs of whitespace (multiple spaces, tabs mixed, or embedded newlines) between tokens. Confirm the intended limits (1-5) cover all real-world inputs and that tests include examples with varying whitespace and newline/tab characters.<br> - [ ] <a href='https://github.com/apache/superset/pull/36983/files#diff-1da4a4c94efded7602098954ebb9829c94680a5ee65ecc6ceb2a0f2282c4ad82R616-R658'><strong>Assertion Weakness</strong></a><br>The test assertions only check `result[0]` (the "since" value) to decide whether the bounded-regex matched. That is brittle because valid parsed ranges may populate only `until` (or vice versa) depending on the input. The test should assert the presence/absence of parsing more explicitly (for example, assert at least one side is non-None for expected matches and assert both are None for expected non-matches) to avoid false positives/negatives.<br> - [ ] <a href='https://github.com/apache/superset/pull/36983/files#diff-1da4a4c94efded7602098954ebb9829c94680a5ee65ecc6ceb2a0f2282c4ad82R616-R658'><strong>Test Robustness / Coverage</strong></a><br>The parametrized inputs exercise many spacing edge cases, but do not assert more granular outcomes (e.g., which side of the range was parsed, or that the bounded regex — not datetime fallback — handled the input). Consider asserting more precise outcomes or adding targeted tests that inspect the underlying regex match result to ensure behavior matches the intended change.<br> - [ ] <a href='https://github.com/apache/superset/pull/36983/files#diff-a3775152bef6d7a152e08f6612e95dec7672bad8d2247fe4e1eeee4996380412R434-R436'><strong>Regex usage / performance</strong></a><br>Patterns are created as raw strings in-place and matched with re.search on every loop iteration. Consider normalizing input whitespace before matching and/or pre-compiling regexes with flags to both avoid accidental mismatches and improve reuse. Also verify that re.search is used intentionally (patterns include ^/$ anchors so re.fullmatch or compiled .fullmatch may be clearer).<br> - [ ] <a href='https://github.com/apache/superset/pull/36983/files#diff-1da4a4c94efded7602098954ebb9829c94680a5ee65ecc6ceb2a0f2282c4ad82R616-R658'><strong>Docstring Mismatch</strong></a><br>The test docstring states "Reject expressions with 0 or 6+ spaces", but some test cases expect 0-space positions to parse successfully (because the regex uses `\s{0,5}` in some places). This mismatch between the test description and parametrized expectations can confuse future maintainers and lead to incorrect test updates.<br> </td></tr> </table> -- 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]
