bito-code-review[bot] commented on code in PR #38200: URL: https://github.com/apache/superset/pull/38200#discussion_r2844423297
########## superset-core/src/superset_core/extensions/constants.py: ########## @@ -0,0 +1,35 @@ +# 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. + +""" +Constants for extension validation and naming. +""" + +# Publisher validation pattern: lowercase letters, numbers, hyphens; must start with +# letter; no consecutive hyphens or trailing hyphens +PUBLISHER_PATTERN = r"^[a-z]([a-z0-9]+(-[a-z0-9]+)*)?$" + +# Technical name validation pattern: lowercase letters, numbers, hyphens; must start +# with letter; no consecutive hyphens or trailing hyphens +TECHNICAL_NAME_PATTERN = r"^[a-z]([a-z0-9]+(-[a-z0-9]+)*)?$" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect regex validation</b></div> <div id="fix"> The TECHNICAL_NAME_PATTERN regex has the same issue as PUBLISHER_PATTERN - it incorrectly rejects valid hyphenated technical names. Update to match the comment description. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion TECHNICAL_NAME_PATTERN = r"^[a-z][a-z0-9]*(-[a-z0-9]+)*$" ```` </div> </details> </div> <small><i>Code Review Run #2fb943</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-core/src/superset_core/extensions/constants.py: ########## @@ -0,0 +1,35 @@ +# 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. + +""" +Constants for extension validation and naming. +""" + +# Publisher validation pattern: lowercase letters, numbers, hyphens; must start with +# letter; no consecutive hyphens or trailing hyphens +PUBLISHER_PATTERN = r"^[a-z]([a-z0-9]+(-[a-z0-9]+)*)?$" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect regex validation</b></div> <div id="fix"> The PUBLISHER_PATTERN regex does not match its comment description. It rejects valid hyphenated names like "a-b" because it requires alphanumeric characters immediately after the starting letter when hyphens are present. The pattern should be updated to correctly validate publisher names. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion PUBLISHER_PATTERN = r"^[a-z][a-z0-9]*(-[a-z0-9]+)*$" ```` </div> </details> </div> <small><i>Code Review Run #2fb943</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-extensions-cli/tests/test_name_transformations.py: ########## @@ -20,381 +20,515 @@ from superset_extensions_cli.exceptions import ExtensionNameError from superset_extensions_cli.utils import ( generate_extension_names, + get_module_federation_name, kebab_to_camel_case, kebab_to_snake_case, name_to_kebab_case, - to_snake_case, # Keep this for backward compatibility testing only - validate_extension_name, + suggest_technical_name, + validate_display_name, validate_extension_id, validate_npm_package_name, + validate_publisher, validate_python_package_name, + validate_technical_name, ) -class TestNameTransformations: - """Test name transformation functions.""" - - @pytest.mark.parametrize( - "display_name,expected", - [ - ("Hello World", "hello-world"), - ("Data Explorer", "data-explorer"), - ("My Extension", "my-extension"), - ("hello-world", "hello-world"), # Already normalized - ("Hello@World!", "helloworld"), # Special chars removed - ( - "Data_Explorer", - "data-explorer", - ), # Underscores become spaces then hyphens - ("My Extension", "my-extension"), # Multiple spaces normalized - (" Hello World ", "hello-world"), # Trimmed - ("API v2 Client", "api-v2-client"), # Numbers preserved - ("Simple", "simple"), # Single word - ], - ) - def test_name_to_kebab_case(self, display_name, expected): - """Test direct kebab case conversion from display names.""" - assert name_to_kebab_case(display_name) == expected - - @pytest.mark.parametrize( - "kebab_name,expected", - [ - ("hello-world", "helloWorld"), - ("data-explorer", "dataExplorer"), - ("my-extension", "myExtension"), - ("api-v2-client", "apiV2Client"), - ("simple", "simple"), # Single word - ("chart-tool", "chartTool"), - ("dashboard-helper", "dashboardHelper"), - ], - ) - def test_kebab_to_camel_case(self, kebab_name, expected): - """Test kebab-case to camelCase conversion.""" - assert kebab_to_camel_case(kebab_name) == expected - - @pytest.mark.parametrize( - "kebab_name,expected", - [ - ("hello-world", "hello_world"), - ("data-explorer", "data_explorer"), - ("my-extension", "my_extension"), - ("api-v2-client", "api_v2_client"), - ("simple", "simple"), # Single word - ("chart-tool", "chart_tool"), - ("dashboard-helper", "dashboard_helper"), - ], - ) - def test_kebab_to_snake_case(self, kebab_name, expected): - """Test kebab-case to snake_case conversion.""" - assert kebab_to_snake_case(kebab_name) == expected - - # Backward compatibility test for remaining legacy function - @pytest.mark.parametrize( - "input_name,expected", - [ - ("hello-world", "hello_world"), - ("data-explorer", "data_explorer"), - ("my-extension-name", "my_extension_name"), - ], - ) - def test_to_snake_case_legacy(self, input_name, expected): - """Test legacy kebab-to-snake conversion function.""" - assert to_snake_case(input_name) == expected - - -class TestValidation: - """Test validation functions.""" - - @pytest.mark.parametrize( - "valid_display", - [ - "Hello World", - "Data Explorer", - "My Extension", - "Simple", - " Extra Spaces ", # Gets normalized - ], - ) - def test_validate_extension_name_valid(self, valid_display): - """Test valid display names.""" - result = validate_extension_name(valid_display) - assert result # Should return normalized name - assert " " not in result # No double spaces - - @pytest.mark.parametrize( - "invalid_display,error_match", - [ - ("", "cannot be empty"), - (" ", "cannot be empty"), - ("@#$%", "must contain at least one letter or number"), - ], - ) - def test_validate_extension_name_invalid(self, invalid_display, error_match): - """Test invalid extension names.""" - with pytest.raises(ExtensionNameError, match=error_match): - validate_extension_name(invalid_display) - - @pytest.mark.parametrize( - "valid_id", - [ - "hello-world", - "data-explorer", - "myext", - "chart123", - "my-tool-v2", - "a", # Single character - "extension-with-many-parts", - ], - ) - def test_validate_extension_id_valid(self, valid_id): - """Test valid extension IDs.""" - # Should not raise exceptions - validate_extension_id(valid_id) - - @pytest.mark.parametrize( - "invalid_id,error_match", - [ - ("", "cannot be empty"), - ("Hello-World", "Use lowercase"), - ("-hello", "cannot start with hyphens"), - ("hello-", "cannot end with hyphens"), - ("hello--world", "consecutive hyphens"), - ], - ) - def test_validate_extension_id_invalid(self, invalid_id, error_match): - """Test invalid extension IDs.""" - with pytest.raises(ExtensionNameError, match=error_match): - validate_extension_id(invalid_id) - - @pytest.mark.parametrize( - "valid_package", - [ - "hello_world", - "data_explorer", - "myext", - "test123", - "package_with_many_parts", - ], - ) - def test_validate_python_package_name_valid(self, valid_package): - """Test valid Python package names.""" - # Should not raise exceptions - validate_python_package_name(valid_package) - - @pytest.mark.parametrize( - "keyword", - [ - "class", - "import", - "def", - "return", - "if", - "else", - "for", - "while", - "try", - "except", - "finally", - "with", - "as", - "lambda", - "yield", - "False", - "None", - "True", - ], - ) - def test_validate_python_package_name_keywords(self, keyword): - """Test that Python reserved keywords are rejected.""" - with pytest.raises( - ExtensionNameError, match="Package name cannot start with Python keyword" - ): - validate_python_package_name(keyword) - - @pytest.mark.parametrize( - "invalid_package", - [ - "hello-world", # Hyphens not allowed in Python identifiers - ], - ) - def test_validate_python_package_name_invalid(self, invalid_package): - """Test invalid Python package names.""" - with pytest.raises(ExtensionNameError, match="not a valid Python package"): - validate_python_package_name(invalid_package) - - @pytest.mark.parametrize( - "valid_npm", - [ - "hello-world", +# Name transformation tests + + [email protected]( + "display_name,expected", + [ + ("Hello World", "hello-world"), + ("Data Explorer", "data-explorer"), + ("My Extension", "my-extension"), + ("hello-world", "hello-world"), # Already normalized + ("Hello@World!", "helloworld"), # Special chars removed + ( + "Data_Explorer", "data-explorer", - "myext", - "package-with-many-parts", - ], - ) - def test_validate_npm_package_name_valid(self, valid_npm): - """Test valid npm package names.""" - # Should not raise exceptions - validate_npm_package_name(valid_npm) - - @pytest.mark.parametrize( - "reserved_name", - ["node_modules", "npm", "yarn", "package.json", "localhost", "favicon.ico"], - ) - def test_validate_npm_package_name_reserved(self, reserved_name): - """Test that npm reserved names are rejected.""" - with pytest.raises(ExtensionNameError, match="reserved npm package name"): - validate_npm_package_name(reserved_name) - - -class TestNameGeneration: - """Test complete name generation.""" - - @pytest.mark.parametrize( - "display_name,expected_kebab,expected_snake,expected_camel", - [ - ("Hello World", "hello-world", "hello_world", "helloWorld"), - ("Data Explorer", "data-explorer", "data_explorer", "dataExplorer"), - ("My Extension v2", "my-extension-v2", "my_extension_v2", "myExtensionV2"), - ("Chart Tool", "chart-tool", "chart_tool", "chartTool"), - ("Simple", "simple", "simple", "simple"), - ("API v2 Client", "api-v2-client", "api_v2_client", "apiV2Client"), - ( - "Dashboard Helper", - "dashboard-helper", - "dashboard_helper", - "dashboardHelper", - ), - ], - ) - def test_generate_extension_names_complete_flow( - self, display_name, expected_kebab, expected_snake, expected_camel + ), # Underscores become spaces then hyphens + ("My Extension", "my-extension"), # Multiple spaces normalized + (" Hello World ", "hello-world"), # Trimmed + ("API v2 Client", "api-v2-client"), # Numbers preserved + ("Simple", "simple"), # Single word + ], +) +def test_name_to_kebab_case(display_name, expected): + """Test direct kebab case conversion from display names.""" + assert name_to_kebab_case(display_name) == expected + + [email protected]( + "kebab_name,expected", + [ + ("hello-world", "helloWorld"), + ("data-explorer", "dataExplorer"), + ("my-extension", "myExtension"), + ("api-v2-client", "apiV2Client"), + ("simple", "simple"), # Single word + ("chart-tool", "chartTool"), + ("dashboard-helper", "dashboardHelper"), + ], +) +def test_kebab_to_camel_case(kebab_name, expected): + """Test kebab-case to camelCase conversion.""" + assert kebab_to_camel_case(kebab_name) == expected + + [email protected]( + "kebab_name,expected", + [ + ("hello-world", "hello_world"), + ("data-explorer", "data_explorer"), + ("my-extension", "my_extension"), + ("api-v2-client", "api_v2_client"), + ("simple", "simple"), # Single word + ("chart-tool", "chart_tool"), + ("dashboard-helper", "dashboard_helper"), + ], +) +def test_kebab_to_snake_case(kebab_name, expected): + """Test kebab-case to snake_case conversion.""" + assert kebab_to_snake_case(kebab_name) == expected + + +# Display name validation tests + + [email protected]( + "valid_display", + [ + "Hello World", + "Data Explorer", + "My Extension", + "Simple", + " Extra Spaces ", # Gets normalized + "Dashboard Widgets", + "Chart Builder Pro", + "API Client v2.0", + "Tool_123", # Underscores allowed + "My-Extension", # Hyphens allowed + ], +) +def test_validate_display_name_valid(valid_display): + """Test valid display names.""" + result = validate_display_name(valid_display) + assert result # Should return normalized name + assert " " not in result # No double spaces Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incomplete test assertion</b></div> <div id="fix"> The test for validate_display_name_valid checks only that the result is truthy and lacks double spaces, but doesn't validate the exact normalized output. This could miss bugs in normalization, such as incorrect whitespace handling. Per testing best practices, assert the computed value directly. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion @pytest.mark.parametrize( "valid_display,expected", [ ("Hello World", "Hello World"), ("Data Explorer", "Data Explorer"), ("My Extension", "My Extension"), ("Simple", "Simple"), (" Extra Spaces ", "Extra Spaces"), # Gets normalized ("Dashboard Widgets", "Dashboard Widgets"), ("Chart Builder Pro", "Chart Builder Pro"), ("API Client v2.0", "API Client v2.0"), ("Tool_123", "Tool_123"), # Underscores allowed ("My-Extension", "My-Extension"), # Hyphens allowed ], ) def test_validate_display_name_valid(valid_display, expected): """Test valid display names.""" result = validate_display_name(valid_display) assert result == expected ```` </div> </details> </div> <small><i>Code Review Run #2fb943</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
