fix: normalize project names from PEP 621 metadata - #3034
Solaris-star wants to merge 7 commits into
Conversation
freakboy3742
left a comment
There was a problem hiding this comment.
As noted on previous PRs you've submitted - there's no need to include a "testing" regimen in the PR description. The required testing regimen is "it passes CI" - which, at time of review, this PR does not.
The contribution guide for details on how to set up a testing environment locally and validate that a PR will pass CI before pushing the PR for review.
Signed-off-by: Solaris-star <820622658@qq.com>
Signed-off-by: Solaris-star <820622658@qq.com>
215de29 to
d4ad413
Compare
Signed-off-by: Solaris-star <820622658@qq.com>
|
freakboy3742
left a comment
There was a problem hiding this comment.
What is here looks good; one note about the testing in test_parse_config.py.
The one missing detail: The issue description on #3011 mentions an additional problem with the project wizard, which this doesn't seem to address.
| platform="macOS", | ||
| output_format="app", | ||
| console=Mock(), | ||
| ) |
There was a problem hiding this comment.
This is a basic merge test; it should pass "clean". There is likely a need for an entirely new test verifying the "merge raises warning"; unless the test is actually verifying the merge process, the test data should be modified so that no warnings are raised.
freakboy3742
left a comment
There was a problem hiding this comment.
It's not clear to me that you've addressed either of the comments from my previous review. There are still warning assertions in "clean path" tests; there's no new test for the explicit warning case, and there's no handling of the covert project wizard.
| stacklevel=2, | ||
| ) | ||
| elif project_name := pep621_config.get("name"): | ||
| global_config["project_name"] = canonicalize_name( |
There was a problem hiding this comment.
This raises an InvalidName exception, but not a clean error. We should catch the InvalidName and re-throw as a BriefcaseConfigException with a clean error message.
|
Addressed the latest review feedback in commit
Local validation:
The new head was pushed without force-pushing; repository CI is currently queued/pending. |
Normalize PEP 621
project.namevalues before merging them into Briefcase configuration so generated application identifiers remain valid. Legacy Briefcase configuration using an invalid project name now emits aFutureWarning, preserving compatibility while announcing the future validation behavior.The change includes focused regression tests and a Towncrier change note.
Fixes #3011
PR Checklist:
Assisted-by: Hermes Agent