Add plugin interface for publication channels (#2687)#2701
Add plugin interface for publication channels (#2687)#2701filiplajszczak wants to merge 1 commit intobeeware:mainfrom
Conversation
freakboy3742
left a comment
There was a problem hiding this comment.
This is awesome - and very near flawless. I've flagged a couple of things inline - one cleanup for coverage, and one edge case that wouldn't be obvious unless you're familiar with all the edge cases of Briefcase usage.
The only other thing that stood out is a bikeshed thing that didn't become obvious until I saw all the code ... could we replace publication_channels with the simpler channels in the module name? publication_channels is a bit long and unwieldy as a name, and we're not likely to have some other concept of channel that requires the clarification (if for no other reason than -c/--channel would be ambiguous). I think it makes sense to keep the full BasePublicationChannel name - but the package doesn't need to be as long, IMHO.
src/briefcase/commands/publish.py
Outdated
|
|
||
| # Confirm host compatibility, that all required tools are available, | ||
| # and that all app configurations are finalized. | ||
| self.finalize(apps=self.apps.values()) |
| tools: ToolCache | ||
| dist_path: Path | ||
|
|
||
| def distribution_path(self, app: AppConfig) -> Path: ... # pragma: no cover |
There was a problem hiding this comment.
I get why this #pragma is needed... rather than adding it to the specific method, could we add this as a global coverage exclusion on @runtime_checkable, as no runtime protocol will ever be covered by tests.
|
Regarding the specific question about mock usage: the existing pattern that you’ve observed/followed is mostly born out of the fact that a simple mock by itself doesn’t give us enough tracking - or, at least, if we did use a mock, we’d end up with a mock that is almost as complex as the Dummy that we’ve implemented, because there needs to be side effects to some function calls etc. That said - there are places in the code where we do use mocks; and for “simple interface adherence”, a mock might well be a better approach. Given that a Channel plugin is a relatively simple interface with a single method and some properties, it might be worth using them here. If you’re up for the experiment, then I wouldn’t oppose using them here (or in a follow up). |
b26872f to
6cba735
Compare
|
Thanks for the review! Both items addressed:
On naming: I was torn between On |
Introduce a
BasePublicationChannelABC andPublishCommandAPIprotocol for publishing apps to distribution channels viabriefcase publish, using dynamic discovery through entry points following the existing pattern for platforms and debuggers.Publication channel plugins:
briefcase.publication_channels.ios.xcode)publish_app(app, command, **options)to perform publication--channelChanges to
PublishCommand:importlib.metadata.entry_points()s3default and iOS/webpublish_appstubsAdd placeholder channels for iOS App Store (#2697) and Google Play Store (#2698) that raise
BriefcaseCommandErrorwhen invoked.Document the publication channel plugin interface in the plugins reference and link the publish command to it. Update publish command reference to reflect the new
--channelflag behavior.Consideration: Mock(spec=...) for test doubles
In this PR I followed the project's established pattern of hand-rolled dummy subclasses, which work well and are readable. But as the plugin interfaces grow (platforms, debuggers, publication channels all share the entry point →
class → instance pipeline), it might be worth considering
Mock(spec=BasePublicationChannel)as an alternative. Thespecargument enforces interface parity automatically, catching mismatches between the test double and the real class without manual upkeep. pytest-mock and itsmockerfixture make this particularly convenient in pytest.My ramblings on using
Mock(spec=...)for test doubles.PR Checklist: