diff --git a/changes/2127.misc.rst b/changes/2127.misc.rst new file mode 100644 index 000000000..55f0f16ac --- /dev/null +++ b/changes/2127.misc.rst @@ -0,0 +1 @@ +Enhance app name validation to include additional checks for format and identifiers diff --git a/src/briefcase/commands/convert.py b/src/briefcase/commands/convert.py index 0a61ab8d5..f66f28efc 100644 --- a/src/briefcase/commands/convert.py +++ b/src/briefcase/commands/convert.py @@ -107,17 +107,12 @@ def input_app_name(self, override_value: str | None) -> str: """ intro = ( "We need a name that can serve as a machine-readable Python package name for " - "your application. This name must be PEP508-compliant - that means the name " - "may only contain letters, numbers, hyphens and underscores; it can't contain " - "spaces or punctuation, and it can't start with a hyphen or underscore." + "your application. The name may only contain letters, numbers, hyphens and underscores; " + "it can't contain spaces or punctuation, and it can't start with a hyphen, underscore or a number." ) default = "hello-world" - if ( - "name" in self.pep621_data - and is_valid_app_name(self.pep621_data["name"]) - and override_value is None - ): + if "name" in self.pep621_data and override_value is None: app_name = canonicalize_name(self.pep621_data["name"]) self.console.divider(title="App name") self.console.prompt() @@ -126,13 +121,36 @@ def input_app_name(self, override_value: str | None) -> str: ) return app_name - if is_valid_app_name(self.base_path.name): # Directory name is normalised - default = canonicalize_name(self.base_path.name) + canonicalized_name = canonicalize_name(self.base_path.name) + # Case 1: Project name is usable as-is (e.g., "foobar", "foo-bar") + if ( + is_valid_app_name(self.base_path.name) + and canonicalized_name == self.base_path.name + ): + # Directory name is already valid - use it directly without prompting + if override_value is None: + self.console.divider(title="App name") + self.console.prompt() + self.console.prompt( + f"Using directory name as app name: {self.base_path.name!r}" + ) + return self.base_path.name + # If override_value is provided, fall through to prompt with it + default = self.base_path.name intro += ( "\n\n" f"Based on your PEP508 formatted directory name, we suggest an " f"app name of '{default}', but you can use another name if you want." ) + # Case 2: Project name could be valid after canonicalization (e.g., "test.name" -> "test-name", "test-app_name" -> "test-app-name") + elif is_valid_app_name(canonicalized_name): + default = canonicalized_name + intro += ( + "\n\n" + f"Based on your directory name, we suggest an " + f"app name of '{default}', but you can use another name if you want." + ) + # Case 3: Project name isn't valid, even after canonicalization - fall back to default return self.console.text_question( intro=intro, diff --git a/src/briefcase/config.py b/src/briefcase/config.py index 6970e7917..6d3662d23 100644 --- a/src/briefcase/config.py +++ b/src/briefcase/config.py @@ -18,26 +18,37 @@ from .constants import RESERVED_WORDS from .exceptions import BriefcaseConfigError -# PEP 508 restricts the naming of modules. The PEP defines a regex that uses -# re.IGNORECASE; but in in practice, packaging uses a version that rolls out the lower -# case, which has very slightly different semantics with non-ASCII characters. This -# definition is the one from -# https://github.com/pypa/packaging/blob/24.0/src/packaging/_tokenizer.py#L80 -PEP508_NAME_RE = re.compile(r"^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9._-]*[a-zA-Z0-9])$") - - -def is_valid_pep508_name(app_name): - """Determine if the name is valid by PEP508 rules.""" - return PEP508_NAME_RE.match(app_name) - def is_reserved_keyword(app_name): """Determine if the name is a reserved keyword.""" - return keyword.iskeyword(app_name.lower()) or app_name.lower() in RESERVED_WORDS + return ( + keyword.iskeyword(app_name.lower()) + or app_name.lower() in RESERVED_WORDS + or keyword.iskeyword(app_name) + ) def is_valid_app_name(app_name): - return not is_reserved_keyword(app_name) and is_valid_pep508_name(app_name) + """Determine if the app name is valid. + + Checks the following: + - It is not a reserved keyword. + - It is a valid Python identifier when hyphens are replaced with underscores. + - It does not start with an underscore (reserved for internal use). + - It does not contain problematic Unicode characters that have edge-case behaviors. + + :param app_name: The app name to validate. + :returns: True if the app name is valid; False otherwise. + """ + module_name = app_name.lower().replace("-", "_") + + return all( + [ + not is_reserved_keyword(app_name), + module_name.isidentifier(), + not module_name.startswith("_"), + ] + ) def make_class_name(formal_name): @@ -400,9 +411,11 @@ def __init__( if not is_valid_app_name(self.app_name): raise BriefcaseConfigError( f"{self.app_name!r} is not a valid app name.\n\n" - "App names must not be reserved keywords such as 'and', 'for' and 'while'.\n" - "They must also be PEP508 compliant (i.e., they can only include letters,\n" - "numbers, '-' and '_'; must start with a letter; and cannot end with '-' or '_')." + "App names must:\n" + "- Not be reserved keywords (like 'and', 'for', 'while', 'main', 'test', etc.)\n" + "- Only contain letters, numbers, hyphens, and underscores\n" + "- Start with a letter (not a number, hyphen, or underscore)\n" + "- Be valid Python identifiers when hyphens are replaced with underscores" ) if not is_valid_bundle_identifier(self.bundle): diff --git a/src/briefcase/integrations/android_sdk.py b/src/briefcase/integrations/android_sdk.py index 5d8942431..bac9308ac 100644 --- a/src/briefcase/integrations/android_sdk.py +++ b/src/briefcase/integrations/android_sdk.py @@ -10,7 +10,6 @@ from datetime import datetime from pathlib import Path -from briefcase.config import PEP508_NAME_RE from briefcase.exceptions import ( BriefcaseCommandError, IncompatibleToolError, @@ -22,12 +21,17 @@ from briefcase.integrations.java import JDK from briefcase.integrations.subprocess import SubprocessArgT +ANDROID_EMULATOR_NAME_RE = re.compile( + r"^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9._-]*[a-zA-Z0-9])$" +) + + DEVICE_NOT_FOUND = re.compile(r"^error: device '[^']*' not found") def create_avd_validator(emulators): def _validate_avd_name(avd): - if not PEP508_NAME_RE.match(avd): + if not ANDROID_EMULATOR_NAME_RE.match(avd): raise ValueError( "An emulator name may only contain letters, numbers, hyphens and underscores." ) diff --git a/tests/commands/convert/test_input_app_name.py b/tests/commands/convert/test_input_app_name.py index be0eede4b..726e75290 100644 --- a/tests/commands/convert/test_input_app_name.py +++ b/tests/commands/convert/test_input_app_name.py @@ -12,21 +12,20 @@ def test_override_is_used(convert_command): def test_no_pep621_data(convert_command, monkeypatch): - """The app directory is used if there is no PEP621-name.""" - mock_text_question = MagicMock() - monkeypatch.setattr(convert_command.console, "text_question", mock_text_question) + """The app directory is used directly if there is no PEP621-name and the directory name is already valid.""" + mock_divider = MagicMock() + mock_prompt = MagicMock() + monkeypatch.setattr(convert_command.console, "divider", mock_divider) + monkeypatch.setattr(convert_command.console, "prompt", mock_prompt) convert_command.base_path /= "test-app-name" - convert_command.input_app_name(None) - mock_text_question.assert_called_once_with( - intro=PartialMatchString( - "Based on your PEP508 formatted directory name, we suggest an app name of 'test-app-name'" - ), - description="App Name", - default="test-app-name", - validator=convert_command.validate_app_name, - override_value=None, - ) + result = convert_command.input_app_name(None) + + assert result == "test-app-name" + mock_divider.assert_called_once_with(title="App name") + assert mock_prompt.call_count == 2 + mock_prompt.assert_any_call() + mock_prompt.assert_any_call("Using directory name as app name: 'test-app-name'") def test_valid_pep621_app_name(convert_command): @@ -72,7 +71,7 @@ def test_hint_is_canonicalized(convert_command, monkeypatch): mock_text_question.assert_called_once_with( intro=PartialMatchString( - "Based on your PEP508 formatted directory name, we suggest an app name of 'test-app-name'" + "Based on your directory name, we suggest an app name of 'test-app-name'" ), description="App Name", default="test-app-name", diff --git a/tests/commands/new/test_validate_app_name.py b/tests/commands/new/test_validate_app_name.py index a7426f4c8..475b16496 100644 --- a/tests/commands/new/test_validate_app_name.py +++ b/tests/commands/new/test_validate_app_name.py @@ -7,7 +7,6 @@ "helloworld", "helloWorld", "hello42world", - "42helloworld", # ?? Are we sure this is correct? "hello_world", "hello-world", ], @@ -24,6 +23,7 @@ def test_valid_app_name(new_command, name): "helloworld!", # contains punctuation "_helloworld", # leading underscore "-helloworld", # leading hyphen + "42helloworld", # starting with a number "pass", # python keyword "Pass", # python keyword, but different case usage "PASS", # python keyword, but all upper case diff --git a/tests/config/test_AppConfig.py b/tests/config/test_AppConfig.py index 82a3eaf40..d23e566c2 100644 --- a/tests/config/test_AppConfig.py +++ b/tests/config/test_AppConfig.py @@ -180,6 +180,8 @@ def test_extra_attrs(): "my_app", # contains underscore "myapp2", # ends with digit "my2app", # contains digit + "myApp-", # ends with hyphen (converts to valid identifier) + "myApp_", # ends with underscore (valid Python identifier) ], ) def test_valid_app_name(name): @@ -204,9 +206,7 @@ def test_valid_app_name(name): "myapp!", # end punctuation "my$app", # other punctuation "-myApp", # initial hyphen - "myApp-", # end hyphen "_myApp", # initial underscore - "myApp_", # end underscore ], ) def test_invalid_app_name(name): diff --git a/tests/config/test_is_valid_app_name.py b/tests/config/test_is_valid_app_name.py index 936824a4f..8f2ec718e 100644 --- a/tests/config/test_is_valid_app_name.py +++ b/tests/config/test_is_valid_app_name.py @@ -9,9 +9,14 @@ "helloworld", "helloWorld", "hello42world", - "42helloworld", "hello_world", "hello-world", + "helloworld42", + "a", # single letter + "abc123", # alphanumeric + "none", # `None` is illegal, but when lower case, it isn't a reserved word + "helloworld_", # ends with underscore (valid Python identifier) + "helloworld-", # ends with hyphen (converts to valid identifier) ], ) def test_is_valid_app_name(name): @@ -22,10 +27,12 @@ def test_is_valid_app_name(name): @pytest.mark.parametrize( "name", [ + # Invalid characters "hello world", "helloworld!", "_helloworld", "-helloworld", + # python reserved words "switch", "pass", "false", @@ -34,12 +41,25 @@ def test_is_valid_app_name(name): "main", "socket", "test", - # ı, İ and K (i.e. 0x212a) are valid ASCII when made lowercase and as such are - # accepted by the official PEP 508 regex... but they are rejected here to ensure - # compliance with the regex that is used in practice. - "helloworld_ı", - "İstanbul", - "Kelvin", + "None", + # Additional invalid formats + "my$app", # dollar sign + "app@domain", # at symbol + "app.name", # period + "2app", # starts with number + "42app", # starts with number + "_", # single underscore + "-", # single hyphen + "1", # single digit + "123", # all digits + # Case variations of reserved words + "Switch", + "SWITCH", + "FALSE", + "Pass", + "PASS", + "Test", + "TEST", ], ) def test_is_invalid_app_name(name):