-
-
Notifications
You must be signed in to change notification settings - Fork 375
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
Add --template-branch to briefcase new #1101
Conversation
Signed-off-by: Nadav Levi <[email protected]>
Signed-off-by: Nadav Levi <[email protected]>
Added description of --template-branch option
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.
Thanks for the PR! The implementation and docs you've got here look great (two minor things flagged inline).
The only thing that is missing is test coverage. I'm guessing the two tests you updated were updated to make the existing tests pass; however, we maintain 100% branch coverage in our test suite, and you've added a branch that isn't being exercised. You can see the coverage report in the logs of the failing CI step; you'll need to add a test that exercises the new functionality to prove that the new flag is actually honoured at runtime. You should be able to copy/modify one of the existing tests in tests/commands/new/test_new_app.py
- you might even be able to extend the existing parameterised test to include a template branch argument.
Co-authored-by: Russell Keith-Magee <[email protected]>
Signed-off-by: Nadav Levi <[email protected]>
Thanks for the detailed feedback! |
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 great - the test is exactly what is needed, and CI is now passing!
One minor tweak: instead of v0.3
, it would be useful to use a custom branch name that "stands out" (e.g., experimental
). There's a lot of tests, and the test setup can be complex; so anything we can do to make the purpose of a test stand out is helpful for whoever next dives into this code.
Ordinarily, I wouldn't worry about going through another PR review cycle for something like this - I'd make the tweak myself and land the patch. However, you've submitted this PR from your main
branch, which means I can submit changes. When you submit your next PR (as I hope you do!), it's really helpful if you make your changes on a branch in your own repo, rather than directly on main
of your fork. That way, I (or other maintainers) can make small modifications before landing. It will also make your life easier, as your main
branch becomes a mirror of the upstream project repository, which makes keeping the two in sync much easier.
(You can also move this PR off Draft mode - it's ready to land!)
Typo correction for @nadi726's benefit: if you submit a PR from your main branch, it means maintainers can't push changes. |
I just assumed that it's better for the branch in the test to be a branch that exists on the template repo, which is why i used |
I can see where that motivation comes from - however, tests should always be isolated from the external environment. If a test requires access to a live filesystem or network connection (or, in this case, a specific repo branch), the test is going to be fragile. In some respects, using a non-existent branch name is a safety catch to ensure that the test is isolated from the external environment.
No problem at all - it's a minor thing, but surprisingly common (especially since Github doesn't prevent it). It won't prevent us from accepting this PR; it just slows down the merge because we can't make small tweaks before landing a PR. |
Hi, any specific reason this hasn't been merged yet? |
@nadi726 My apologies - there appears to have been some confusion. If you've changed the name of the branch, that hasn't been reflected in Github - it's still showing this branch as a PR from |
Ah, no, sorry for the ambiguity - that's not what i meant.
|
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.
Ah! I'm so sorry - I completely missed that you had pushed that update.
This looks great! Thanks for the contribution!
Added
--template-branch
tobriefcase new
.Solves #1053.
In accordance,
new
command reference in the docs.*Except the coverage problem that the automatic tests show. Don't know how to implement this.
PR Checklist: