codeant-ai-for-open-source[bot] commented on PR #37039: URL: https://github.com/apache/superset/pull/37039#issuecomment-3736832767
## 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/37039/files#diff-fda2a3496e8791d837d7a931dbb9d00e4fdcaf6e88447ed9a2c10d383d5c7fb5R66-R82'><strong>Display Bug</strong></a><br>The new `normalizeForCurrency` strips '%' from the formatted output. Because `%` is now preserved in the d3 format (so the numeric semantics are correct), removing the percent character from the final string will drop the visible percent sign (e.g. "12.3%" → "12.3"), which is likely not the intended UX. Consider whether the percent symbol should remain visible when a currency prefix/suffix is applied, or only be removed in very specific cases.<br> - [ ] <a href='https://github.com/apache/superset/pull/37039/files#diff-3acb2ccc80943db365b5dccc39049274584f9080e7dbc988db50b49d3a4ba8deR108-R114'><strong>D3 '%' semantics</strong></a><br>The PR preserves the '%' character in the d3 format string so d3's percentage semantics (multiplying the numeric value by 100) remain intact. Verify the implementation doesn't accidentally mutate the d3 format in other code paths (e.g., other format tokens or combined currency+D3 strings) and that getNormalizedD3Format consistently returns the original d3 format when '%' is present.<br> - [ ] <a href='https://github.com/apache/superset/pull/37039/files#diff-3acb2ccc80943db365b5dccc39049274584f9080e7dbc988db50b49d3a4ba8deR149-R154'><strong>Edge cases not covered</strong></a><br>There are no tests for fractional values, negative numbers, or small values when using percentage formats and currency prefix/suffix. These edge cases can surface rounding or sign-placement issues; consider adding tests for them.<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]
