Fix app name validation logic and improve error message #2405

egarciamendez · 2025-07-20T09:30:58Z

This PR fixes a bug in the is_valid_app_name function and improves the error message for better user experience.

Changes Made

Fixed Logic Bug in is_valid_app_name

Corrected the validation logic that was incorrectly using is_reserved_keyword(app_name)
Added explicit validation for the string 'None' which should be rejected as an app name
Updated the function docstring to accurately reflect all validation checks

Enhanced Error Message

Replaced the single-paragraph error message with a clear, structured bullet-point format
Added specific validation rules that users can easily follow:
- Not be reserved keywords (with examples)
- Be PEP508 compliant
- Start with a letter
- Not end with hyphen or underscore
- Be valid Python identifiers when hyphens are replaced with underscores

Comprehensive Test Coverage

Added extensive test cases to test_is_valid_app_name.py covering:
- Additional Python keywords (and, or, if, def, class, etc.)
- Case variations of reserved words (Switch, SWITCH, Pass, PASS)
- Invalid formatting scenarios (starts with number, ends with underscore/hyphen)
- Edge cases (empty string, single characters)
Added dedicated test function test_reserved_keyword_logic_fix() to specifically verify the logic fix
Enhanced existing test coverage with more valid app name examples

Problem Solved

Apps with reserved keyword names causing runtime issues
Valid app names being incorrectly rejected
Confusing error messages for developers

The improved error message provides clear, actionable guidance to developers when their app name is rejected.

Fixes #2127

PR Checklist:

All new features have been tested
All new features have been documented
I have read the CONTRIBUTING.md file
I will abide by the code of conduct

…nd identifiers

egarciamendez · 2025-07-20T09:50:05Z

Tests failed :-( i will be looking into those when I'm back home

…with a number

…ackage names

egarciamendez · 2025-07-21T11:04:08Z

Hi @freakboy3742 ! I'm getting some errors that are unrelated to my changes. At least I suppose so. Could you check those out please?

freakboy3742 · 2025-07-22T00:41:52Z

@egarciamendez Looks like you were bitten by some transient network problems - not sure if the problem was on Github's side, or Adoptium's - but either way, re-running the tests looks like it has succeeded.

This indicates there are some tests that we should probably clean up - there's very limited situations where we should be dependent on external network availability to run the test suite, and from the look of these test failures, none of these would qualify. I've added #2409 to track this issue with the test suite.

freakboy3742

This is definitely headed in the right direction - a couple of cleanups and suggestions inline.

docs/about/releases.rst

src/briefcase/config.py

src/briefcase/constants.py

tests/commands/convert/test_input_app_name.py

tests/config/test_is_valid_app_name.py

Co-authored-by: Russell Keith-Magee <[email protected]>

…eck for valid module name

freakboy3742

This revision has overcorrected a little too much - it's allowing names that shouldn't be allowed, and rejecting names that should be valid. To be fair - some of those rejected names were historically rejected as well - so this is at least partially a case of the existing test cases being misleading.

I think the core of the problem is that PEP 508 compliance in the app name is a red herring. Despite the historical code, we don't actually want PEP 508 compliance for app names - we want "is valid identifier after replacing - with _" and "is not a keyword".

The convert wizard then needs different handling, because project.name is governed solely by PEP 508 - but there are valid PEP508 names that aren't valid app names (e.g., foo.bar), and names that aren't valid PEP 508 names, but are validated app names (e.g., foo_). In the convert wizard, there's essentially 2 cases:

The project name is a valid app name as written
The project name can be canonicalised into a valid app name.

Case 1 is the simple case (e.g., project.name = "foobar") - in which case, the name is being used unmodified, so we can use it as the app name without any confirmation.

Case 2 is the slightly more complex case, where we are applying a transformation (e.g., project.name = "foo.bar", which canonicalises to foo_bar). In this case, that's likely a good candidate, but we need to get the user to confirm this.

If neither of these cases are true - 1foo.bar, for example - then we need to fall back to alternate suggestions (such as using the directory name).

Does that make sense?

src/briefcase/config.py

freakboy3742 · 2025-07-25T00:04:40Z

src/briefcase/commands/convert.py

-            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:


I'm not sure why you've dropped the validity check here - 1hello.world is a valid project.name by PEP 508, which canonicalizes to 1hello-world - but that's not a valid app name, so it can't be used as a candidate.

I dropped it because otherwise I wouldn't be able to pass this test:

test.name from the [project] name in pyproject.toml isn’t a valid app name and therefore won’t be canonicalized to test-name.

To be honest, I’m a bit confused right now about what I should do instead.

There's 3 cases here:

The project name is useable as-is. foobar and foo-bar are both valid project names, and are also valid app names as-is. They can be directly provided as the candidate app name, as there's no transformation being applied.

The project name could be a valid name, but requires some canonicalisation. test.name is a valid PEP 508 project name, and it canonicalizes to test-name; we don't want to provide that name as an automatic app name, but it's a reasonable candidate for an app name that the user could confirm as valid.

The project name isn't valid, even after canonicalisation. 42foobar is a valid project name - but it canonicalizes to exactly the same value, which isn't a valid app name. In this case, we need to fall back to other hinted options (such as the base path of the project).

The code has a branch for 1 and 3; but the logic that triggers 1 is incorrect and it merges in the canonicalisation required by case 2. There's no logic handling the case of "test.name" being coerced into a hint for the user to confirm.

Does that make more sense?

Yes, thank you for the explanation (and your patience 😅)

Ok - but my the original point still stands: you've dropped the validation check on this branch. By this code, if user provides any value for name in the [project] data, the canonicalised version of that name will be used. You've added code for the three cases that I described - but this check comes before those cases are evaluated.

src/briefcase/config.py

src/briefcase/constants.py

tests/commands/convert/test_input_app_name.py

…ation

src/briefcase/config.py

freakboy3742 · 2025-08-06T00:30:15Z

src/briefcase/commands/convert.py

-            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:


There's 3 cases here:

The project name is useable as-is. foobar and foo-bar are both valid project names, and are also valid app names as-is. They can be directly provided as the candidate app name, as there's no transformation being applied.

The project name could be a valid name, but requires some canonicalisation. test.name is a valid PEP 508 project name, and it canonicalizes to test-name; we don't want to provide that name as an automatic app name, but it's a reasonable candidate for an app name that the user could confirm as valid.

The project name isn't valid, even after canonicalisation. 42foobar is a valid project name - but it canonicalizes to exactly the same value, which isn't a valid app name. In this case, we need to fall back to other hinted options (such as the base path of the project).

The code has a branch for 1 and 3; but the logic that triggers 1 is incorrect and it merges in the canonicalisation required by case 2. There's no logic handling the case of "test.name" being coerced into a hint for the user to confirm.

Does that make more sense?

…r rules and exclude problematic Unicode characters

…ases for valid identifiers

…s file for permissions

freakboy3742

Thanks for the update; there's still some issues with the validation logic. I've tried to explain inline. Let me know if you need any further clarifications.

tests/config/test_is_valid_app_name.py

freakboy3742 · 2025-09-08T23:56:16Z

src/briefcase/commands/convert.py

-            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:


Ok - but my the original point still stands: you've dropped the validation check on this branch. By this code, if user provides any value for name in the [project] data, the canonicalised version of that name will be used. You've added code for the three cases that I described - but this check comes before those cases are evaluated.

freakboy3742 · 2025-09-08T23:56:45Z

src/briefcase/config.py

 # 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])$")


This regex isn't being used any more, so it can be dropped.

Actually it is being used in some android sdk dependency. That's why I didn't delete it.

Ah - I somehow missed that usage in my searching.

In this case, I think that's a case where the regex can be moved and renamed. It's being used to identify valid Android emulator names, which aren't bound by PEP 508 - they just happen to have very similar rules.

Interestingly, PEP 508 will allow bee.phone as a name, and that works with the emulator (I just checked), but period isn't included in the description of a valid emulator name.

So - I'd suggest moving it to the android module where it's being used, and rename it to ANDROID_EMULATOR_NAME_RE.

src/briefcase/config.py

src/briefcase/commands/convert.py

freakboy3742 · 2025-09-09T00:04:56Z

src/briefcase/commands/convert.py

+            )
+        # 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


By this logic, case 1 and case 2 are functionally identical. The key difference between case 1 and case 2 is that we don't need to ask the user to confirm case 1 - we're using exactly the same name, so it's fine as is. Case 2 involves a transformation, so it needs to be confirmed.

Co-authored-by: Russell Keith-Magee <[email protected]>

… valid

freakboy3742 · 2025-09-22T06:53:05Z

Are you waiting on a review of this code? Aside from the CI pre-commit failure, it looks like some of my comments from my previous review are outstanding. When you're ready for another review, let me know and I'll take a look.

egarciamendez added 2 commits July 20, 2025 11:25

Enhance app name validation to include additional checks for format a…

7c6cd96

…nd identifiers

Enhance app name validation to include additional checks for format a…

cb8d9ee

…nd identifiers

egarciamendez added 4 commits July 21, 2025 12:03

Update app name validation tests to include cases for names starting …

e5727c7

…with a number

Update validation message to clarify naming restrictions for Python p…

f011119

…ackage names

dot in app name is not allowed anymore

982d2ec

Remove 'None' from invalid app name checks and add 'none' to constants

02509a5

freakboy3742 mentioned this pull request Jul 22, 2025

Test suite requires active network connection #2409

Closed

freakboy3742 requested changes Jul 22, 2025

View reviewed changes

egarciamendez and others added 9 commits July 22, 2025 06:59

Update config.py

9d25586

Co-authored-by: Russell Keith-Magee <[email protected]>

ran towncrier by mistake

925280f

remove redudant test cases

7ce356d

remove redundant test cases

615d177

remove redundant test cases

aebdaab

Add 'None' and 'none' to invalid app name test cases

43ee262

Update test case to use 'test.name' for app name canonicalization

aab7c2b

Replace dashes with underscores in canonicalized app name + remove ch…

cc36b4d

…eck for valid module name

Remove dashes replacement from canonicalized app name in convert.py

22c50b7

egarciamendez requested a review from freakboy3742 July 24, 2025 21:05

freakboy3742 requested changes Jul 25, 2025

View reviewed changes

egarciamendez added 3 commits August 5, 2025 13:19

Remove checks for starting with a number and period in app name valid…

cea0991

…ation

Update app name validation message to clarify PEP508 compliance

37eae5e

none is daft but legal

820b0b5

freakboy3742 requested changes Aug 6, 2025

View reviewed changes

egarciamendez added 4 commits September 8, 2025 11:38

Enhance app name suggestion logic based on PEP508 compliance

fed1fe3

Refine app name validation to ensure compliance with Python identifie…

bdb07a8

…r rules and exclude problematic Unicode characters

Refactor PEP508 name validation by removing regex and updating test c…

79851d8

…ases for valid identifiers

Add PEP508 compliant regex for module naming and create local setting…

418a77a

…s file for permissions

egarciamendez requested a review from freakboy3742 September 8, 2025 12:06

freakboy3742 requested changes Sep 9, 2025

View reviewed changes

egarciamendez and others added 8 commits September 9, 2025 23:23

Update test_is_valid_app_name.py

4495ab4

Co-authored-by: Russell Keith-Magee <[email protected]>

Replace PEP508 regex with Android emulator name regex for validation

849bc65

Clarify PEP508 compliance requirements in application name guidelines

05f9444

Improve app name suggestion logic by using directory name directly if…

29d2aa9

… valid

Remove invalid Unicode characters from app name validation tests

a4b4704

Enhance app name suggestion logic to use directory name directly when…

973bcc0

… valid

Enhance app name suggestion logic to use directory name directly when…

3957298

… valid

Enhance app name suggestion logic to use directory name directly when…

32e4277

… valid

Uh oh!

Fix app name validation logic and improve error message #2405

Are you sure you want to change the base?

Fix app name validation logic and improve error message #2405

Uh oh!

Conversation

egarciamendez commented Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist:

Uh oh!

egarciamendez commented Jul 20, 2025

Uh oh!

egarciamendez commented Jul 21, 2025

Uh oh!

freakboy3742 commented Jul 22, 2025

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freakboy3742 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freakboy3742 commented Sep 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

egarciamendez commented Jul 20, 2025 •

edited

Loading