-
-
Notifications
You must be signed in to change notification settings - Fork 473
Add briefcase config Command for User-Level and Global Configuration #2494
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…thod stubs to ConfigCommand
…) used in the new wizard; add a function to write .briefcase directory in .gitignore
… errors; allow '?' sentinel for device/identity keys
…and xcode platform
freakboy3742
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to be broadly on the right track. There's some work needed on the documentation, but the documentation that is there covers all the core details, and it's probably easier for someone closer to the project to do any rewrites to keep the "voice" consistent.
There's also an implied need for a documentation addition - iOS.device and android.device are both properties that need to be added to those backends respective documentation, as based upon this PR, they could be set directly on an app configuration rather than going through global or local configuration.
The other details that is a high level concern is the implemenation of the merge process itself. As flagged inline, the merge process is surprisingly disconnected from the existing config parsing code. There's a lot of seemingly duplicated functionality - briefcase.config already has a merge_config() method; and at the point this block of code is running, we know what platform we're running on. It's not entirely clear to me why new tooling is needed, It seems like much of the functionality for loading and merging should be usable as-is - or, if it can't be, that this is an indicator of a need to refactor the existing tooling.
It also seems like configuration like this should come before parse_config, and be passed into that process as a starting point that the global config is then merged over. It's possible I might be missing something because I'm not actively working on the code, and I've thus missed (or forgotten) the relevant context; if that's the case, let me know.
I haven't reviewed the test code yet - from a quick scan, it looks comprehensive, although some of the test naming seems inconsistent with established patterns. We name test files to track back to the methods that they're testing, rather than their purpose. There's also a lot of duplicated code that feels like it should be in utility classes (or wouldn't be duplicated if the tests were organised by the method they are testing). I'm also surprised that a one line change to device selection requires the addition of 5 additional test files - that seems like the granularity of the tests might be wrong. However, I'll hold off doing a full review of the tests until we know the tests are testing the right thing.
| $ briefcase config --get android.device | ||
| $ briefcase config --global --get iOS.device | ||
| List the current file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| List the current file: | |
| List the current configuration: |
| if sys.version_info >= (3, 11): # pragma: no-cover-if-lt-py311 | ||
| pass | ||
| else: # pragma: no-cover-if-gte-py311 | ||
| pass | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a complete no-op?
| return overrides | ||
|
|
||
|
|
||
| def _ensure_gitignore_briefcase(app_path: Path) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be auto-modifying .gitignore. I've opened beeware/briefcase-template#214 to add .briefcase to the default project.
| # Allow "?" to force interactive selection, even if a value was provided | ||
| if isinstance(device_or_avd, str) and device_or_avd.strip() == "?": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the strip() needed here? And if not - does that mean the whole check can simplify to:
| # Allow "?" to force interactive selection, even if a value was provided | |
| if isinstance(device_or_avd, str) and device_or_avd.strip() == "?": | |
| # Allow "?" to force interactive selection, even if a value was provided | |
| if device_or_avd == "?": |
| if isinstance(device_or_avd, str): | ||
| device_or_avd = device_or_avd.strip() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What case is the strip() catching here? Won't config strings always be precisely terminated?
| android_section = getattr(app, "android", None) | ||
| if isinstance(android_section, dict): | ||
| configured = android_section.get("device") | ||
| else: | ||
| configured = getattr(android_section, "device", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggests to me to that the structure isn't quite right - it says app.android could be either a dictionary or a Config object. It should be one or the other.
By my read, if we're collapsing configurations correctly, on an Android app, app.device should be set (assuming it's defined at either the global config, project config, or app config level)
| raise BriefcaseCommandError( | ||
| f"Unable to find device or AVD '{device_or_avd}'." | ||
| ) from e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this additional level of exception handling needed? InvalidDeviceError is a BriefcaseCommandError.
| for the selected device. | ||
| """ | ||
| # Allow "?" to force interactive selection, even if a value was provided | ||
| if isinstance(udid_or_device, str) and udid_or_device.strip() == "?": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with Android - should this just be
| if isinstance(udid_or_device, str) and udid_or_device.strip() == "?": | |
| if udid_or_device == "?": |
| if udid is None: | ||
| ios_section = getattr(app, "iOS", {}) | ||
| if isinstance(ios_section, dict): | ||
| configured = ios_section.get("device") | ||
| else: | ||
| configured = getattr(app, "device", None) | ||
| if configured: | ||
| udid = configured.strip() or None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with Android - is this not app.device?
| merged_app_configs = { | ||
| name: deep_merge(copy.deepcopy(user_merged), cfg) | ||
| for name, cfg in app_configs.items() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general approach you've taken here looks about right; however, it concerns me the extent to which the config handling is completely dissociated from the rest of the pyproject.toml config handling.
Summary
This PR introduces a new Briefcase
configcommand to allow setting and persisting configuration values at both project-specific and global per-user levels. This enables users to define default preferences without modifyingpyproject.toml.This is a second PR attempt for #2279. There was a previous attempt in #2328; because this version adopts a different approach (centralized merge + platforms consume defaults only when the CLI doesn’t specify), I created a new branch/PR.
Currently, many preferences must be hardcoded in
pyproject.tomlor passed on every invocation. This PR addresses the following gap:briefcase runwill use when--deviceis not provided.Features
New CLI Command
briefcase configmanages user-level configurations. By default it writes to the current project; use--globalfor setting per-user global configurations.Supported operations
briefcase config KEY VALUE— setbriefcase config --get KEY— getbriefcase config --unset KEY— unsetbriefcase config --list— list the current config file--global— target the global per-user config instead of the projectExamples
Configuration precedence
Resolved once during command initialization:
CLI overrides > pyproject.toml > project user config > global user configStorage locations
<project>/.briefcase/config.toml~/Library/Application Support/org.beeware.briefcase/config.tomlon macOS)New projects ensure
.gitignorecontains.briefcase/to avoid committing per-user files.Supported keys
android.device—@AVD(e.g.,@Pixel_5) oremulator-####iOS.device— UDID (xxxxxxxx-…-xxxxxxxxxxxx), device name (e.g.,"iPhone 16"), or"Name::iOS X[.Y]"Special value:
?is allowed for both keys to force interactive selection next run.Not included
author.name/author.email(wizard does not read them).macOS.identity).PR Checklist: