Add guidance for third-party bootstraps#2640
Add guidance for third-party bootstraps#2640moondial-pal wants to merge 35 commits intobeeware:mainfrom
Conversation
|
Hello! Thank you for the contribution. The majority of the failing tests are due to an issue unrelated to your PR. The fix has been submitted and merged into Please be aware that you'll also need to upgrade |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for the PR; however, from the look of it, this doesn't actually address the problem that was described in the ticket.
This PR does add another menu item for "other frameworks"; however, it doesn't provide any real information beyond that. The menu prompt says "select to see options"; the implication of this is that you will... see options. As currently implemented, all you get is text that tells you other options might exist, which is not really any better than the prompt that is already provided.
The purpose of the feature request is to help resolve the discovery issue - to let people know that there are other options they could install, and how to install them.
For example, right now, Toga Positron is a plugin option. If you select option 6, that option should be listed; if you select it, you should be told to install toga-positron.
If toga-positron is installed, that option shouldn't appear in "other options".
The expectation would be that we'd accept contributions from third-parties that want their plugins listed, and we prime that list with the plugins that are already listed in documentation.
Let me know if you need any more clarification on this.
|
Understood, thanks for the feedback. |
- Insert "Other frameworks…" as second-to-last bootstrap choice - Handle sentinel selection with disclaimer and guidance - Abort cleanly instead of indexing into non-existent bootstrap
- Verify selecting Other frameworks aborts as expected - Update nemeric selections to reflect new menu option
Update the test to assert the correct type and compute the menu dynamically to avoid odering assumptions.
- Installed plugins will affect the GUI bootstrap menu - Update the test to select the sentinel entry by value not by hard-coded position
moondial-pal
force-pushed
the
briefcase-bootstraps
branch
from
fb2c2ad to
78c21dd
Compare
|
@kattni Thanks for the heads up! I’ve rebased the branch onto the current main as suggested and confirmed the full test suite passes locally. I’m still seeing CI failures on the PR, so I’m taking another look in case there’s anything else I missed, please let me know if you spot anything on your end. |
|
It’s my understanding that community GUI bootstraps are considered installed I tested both PPB and pygame-ce locally: pygame-ce was detected as installed |
Your understanding is correct; and yes, it looks like PPB isn't actually publishing a bootstrap at this point. I'll need to follow up with the PPB team; IIRC, the entry was added during (or related to activity at) PyCon US sprints; it's possible the PR wasn't finalized or merged. If it doesn't work, there's no point leaving it on the list. |
|
Got it. I'll go ahead and remove ppb from the list for this PR. |
Might as well remove it from the docs as well - if it doesn't work, there's no point highlighting it. |
|
@freakboy3742 Sorry I fat fingered the request for review. I had not made the requested change yet. Apologies.
Thanks again for the guidance, ready for re-review when you are. |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Generally looks good; the review comments are mostly about user ergonomics, and the exact presentation of error message.
The one bigger question is about the overall workflow. The GUI framework question has always been the last question; however, with this new question, it means that you answer all the questions about your app... and then get told to re-run briefcase new. I think it would make sense to make the framework question the first question, so that the user doesn't lose anything if the wizard needs to be re-run.
src/briefcase/commands/new.py
Outdated
| raise BriefcaseCommandError( | ||
| "Install a community GUI bootstrap plugin and re-run `briefcase new`." | ||
| ) |
There was a problem hiding this comment.
Raising BriefcaseCommandError means the command exits as an error, generating a log. Selecting a different GUI framework is "normal behavior", so it shouldn't be generating a log; and while we need to draw the attention of the user, the user hasn't done anything wrong, so it shouldn't surface in red as an error.
I'd suggest once the user selects a plugin, the text should be displayed as a warning, and read something like:
Pygame-ce is provided by a community plugin. To use this plugin, run:
python -m pip install pygame-ce
then re-run `briefcase new`.
I'd also suggest that the output should all be part of the "other frameworks menu" code - if only because the case where you have installed all known plugins, you currently get an "there are no additional plugins; please install a plugin" message.
src/briefcase/commands/new.py
Outdated
| for name in reversed(("Toga", "PySide6", "Pygame")): | ||
| ordered.move_to_end(name, last=False) |
There was a problem hiding this comment.
Why this change? You've taken three lines of explicit code, and used a pre-computed list... which is then processed into reverse order. Why not provide the list in reversed order?
src/briefcase/commands/new.py
Outdated
| self.console.warning() | ||
| self.console.warning( |
There was a problem hiding this comment.
We generally don't use console commands as "one per line of output" - we combine them all into a single call that includes any required newlines.
|
Also - we generally don't rebase PRs unless something catastrophic goes wrong. We use squash commits, so multiple merge commits or noisy "no really fixed this time" commits in a PR won't appear in the final product; and when you rebase, any comments on rebased lines get lost. |
|
I really appreciate the patience and time taken for guidance on these reviews. |
|
Marking this as draft while I finish simplifying the selection logic and aligning the tests with the updated behavior. I’ll move it back to ready once everything is clean and passing. |
|
Working on CI fixes. |
|
Apologies for the issues with the docs builds - we launched a new version of the website on Friday, and we've been shaking out loose links ever since. I think we've got most of them sorted out at this point; but some of the docs build problems require merging with main, so if you're seeing problems, try that as a fix first. If you're still seeing problems, let us know and we can dig into the problem for you. |
Merging upstream main to update branch with changes to docs.
|
Hi @freakboy3742, this is ready for review. I believe I have addressed your feedback and the diff is larger than usual given the new community framework wizard behavior we discussed. Let me know if anything needs adjusting. |
|
@moondial-pal Thanks for the update - I hope to be able to give this a review in the next couple of days. |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for those updates. I think the broad structure is getting closer; but there's some things that are still odd about data structure use, and the specifics of the experience the user has when they run the wizard.
src/briefcase/commands/new.py
Outdated
|
|
||
| # A plugin is treated as "installed" if its distribution package is installed, | ||
| # regardless of how many Briefcase entry points it provides. | ||
| KNOWN_COMMUNITY_PLUGINS: ClassVar[list[dict[str, str]]] = [ |
There was a problem hiding this comment.
This data structure is only ever accessed by iteration, or as lookup for a package name. Why not store it as a dictionary keyed by package name?
src/briefcase/commands/new.py
Outdated
| if plugin.get("description") | ||
| else plugin["display_name"] |
There was a problem hiding this comment.
We're in control of the list of plugins here, so we can guarantee they all have display names and descriptions.
src/briefcase/commands/new.py
Outdated
| bootstrap_class = self.select_bootstrap(project_overrides) | ||
| context = self.build_app_context(project_overrides) |
There was a problem hiding this comment.
This changes the order of questions - which is what we wanted - but doesn't address the fact that the first question on the "build_app_context" question list starts "First, we need ...". The question text will need to be tweaked.
src/briefcase/commands/new.py
Outdated
| if not is_package_installed(plugin["package"]) | ||
| ] | ||
|
|
||
| if not not_installed_plugins: |
There was a problem hiding this comment.
not not_installed is a bit awkward; not uninstalled... would read a bit better.
src/briefcase/commands/new.py
Outdated
| preferred = ["Toga", "PySide6", "Pygame", "Console"] | ||
|
|
||
| # Sort framework names (excluding sentinel options). | ||
| ordered = sorted( | ||
| name | ||
| for name in bootstrap_names | ||
| if name not in (self.OTHER_FRAMEWORKS, "None") | ||
| ) | ||
|
|
||
| # Ensure the first 3 options are: Toga, PySide6, Pygame | ||
| ordered.move_to_end("Pygame", last=False) | ||
| ordered.move_to_end("PySide6", last=False) | ||
| ordered.move_to_end("Toga", last=False) | ||
| # Pull preferred items to the front (in explicit order) if present. | ||
| ordered = [name for name in preferred if name in ordered] + [ | ||
| name for name in ordered if name not in preferred | ||
| ] | ||
|
|
||
| # Option None should always be last | ||
| ordered.move_to_end("None") | ||
| ordered.append(self.OTHER_FRAMEWORKS) | ||
| if "None" in bootstrap_names: | ||
| ordered.append("None") |
There was a problem hiding this comment.
This is now the third time I've provided this feedback (Ref 1, ref 2): why are you changing this section of code?
Unless I'm missing something, the existing code does everything that is needed, and is a lot less complicated. The updated implementation involves multiple sorts, appended lists, and optional comparisons... when all we need is what is already here - sort the inputs, ensure 3 specific options are first, and "Other" and "None" are last. This doesn't require a complex sorting. The code that is already here does everything that is needed.
If there's a something that I'm missing here, please let me know - but otherwise, please revert this to the original code. It works and is easy to understand. It doesn't need to be fixed unless there is actually something wrong with it.
|
Understood, I'll revert the ordering section and address the other feedback. Thanks for your patience. |
|
Checking what may have caused failures on my end. |
|
The pre-commit checks passed locally but the CI caught a line length issue among other things. Circling back to make sure test coverage is 100%. |
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
|
Great point, I can see how that reads ambiguously. Working on getting the platform tests green...I think my dev environment got a bit tangled working across two machines. |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
We're definitely getting closer. Most of the implementation now looks good; I've pushed a couple of cosmetic tweaks to the actual message text, but the basic flow and implementation now makes sense.
The one part that needs to be cleaned up now is an artefact of the refactor, plus the testing that flows on from that.
In the old code, the interface to bootstraps was create_bootstrap(), which handled both selection and instantiation of the bootstrap. This PR has split that up - which makes sense... but the API for create_boostrap()... which is then never called - and the testing is still focussed around calling create_bootstrap().
If a method isn't being used, we don't need to retain it :-)
On top of that, _instantiate_bootstrap() is 2 line implementation of fairly obvious logic that isn't re-used anywhere. I think it's safe to put that content directly into the single place it's used.
This means that the content that is currently in test_create_bootstrap() should really be in a test_select_bootstrap() method - this would be consistent with the fact that most of the test cases in test_create_bootstreap() were really tests of the selection process, not the instantiation process.
I'd suggest that instead of introducing a new _instantiate_boostrap() method, we repurpose the existing create_bootstrap() name to fill that purpose - that is rename _instantiate_bootstrap() to create_bootstrap().
This means a change to tests as well. test_creat_splitting test_create_bootstrap()into two parts - one testingtest_select_bootstrap() to test the selection process, and test_create_bootstrap() - since that capability was previously being tested
I've pushed an update doing the API simplification; however, this means the tests will now fail. If you update the tests to match what I've described here, we should be good to go
|
The continued guidance is much appreciated. I'll get to work on the changes. |
3 participants
Add an “Other frameworks…” option to the GUI bootstrap selection shown during
briefcase new.Users can now explicitly select a community provided GUI framework option,
receive guidance about support expectations, and exit cleanly with next steps.
Fixes #2342
PR Checklist: