Conversation
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
At a high level, this is doing the right thing. A couple of comments inline about the specific changes you've made - most notably, deleting previously existing documentation. Unless existing documentation is fundamentally wrong or misleading, you should be very hesitant to delete existing comments.
Also - this change can be applied more broadly than just the create command. The changes you've made for change look good; but every command will be invoking finalize(), and will need comparable changes.
src/briefcase/commands/base.py
Outdated
| from collections.abc import Iterable | ||
|
|
There was a problem hiding this comment.
This import definitely shouldn't be here - and it's redundant anyway because it's at the top of the file.
| from collections.abc import Iterable |
src/briefcase/commands/base.py
Outdated
| def finalize( | ||
| self, | ||
| app: AppConfig | None = None, | ||
| apps: AppConfig | Iterable[AppConfig] | None = None, |
There was a problem hiding this comment.
We should be able to exclude None and a bare AppConfig from this list by changing the places where it is used.
src/briefcase/commands/base.py
Outdated
| :param debugger_host: The host to use for the debugger | ||
| :param debugger_port: The port to use for the debugger | ||
| """ | ||
| """Finalize Briefcase configuration.""" |
There was a problem hiding this comment.
Why have you deleted this docstring? It definitely needs to be updated, but the content is still useful.
|
|
||
| if app.external_package_path: | ||
| # Package path is defined | ||
| if app.sources is not None: |
There was a problem hiding this comment.
Why have you deleted these docstrings? They're still accurate and helpful.
|
I’ve applied the All tests pass, and I’ve manually verified behavior with multiple apps in a local project (default invocation and -a / --app cases). |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Looks almost perfect - I picked up one small edge that looks incorrect.
|
|
||
| # Confirm host compatibility, that all required tools are available, | ||
| # and that the app configuration is finalized. | ||
| self.finalize(apps=self.apps.values()) |
There was a problem hiding this comment.
This should only be finalizingthe apps being packaged:
| self.finalize(apps=self.apps.values()) | |
| self.finalize(apps=apps_to_package.values()) |
| ("verify-tools",), | ||
| # App config has been finalized | ||
| ("finalize-app-config", "first"), | ||
| ("finalize-app-config", "second"), |
There was a problem hiding this comment.
This is likely the side effect of the change flagged in the package command. This test is explicitly packaging a single app; only one app should be finalized.
2 participants
When using the
-a/--appoptions, Briefcase no longer finalizes all apps in a project.This ensures that only the selected app is finalized, preventing configuration errors from unrelated apps.
Fixes #2654
PR Checklist: