Closed sarayourfriend closed 1 month ago
Ahh, str::capitalize
won't work, because it lowercases the rest of the string, and ignores sentences:
>>> "Hello, World. Briefcase is Cool.".capitalize()
'Hello, world. briefcase is cool.'
I'll switch back to upper
on the first character, plus the other improvements you made :+1: Should have time to do that in the next couple of hours.
Ahh,
str::capitalize
won't work, because it lowercases the rest of the string, and ignores sentences:>>> "Hello, World. Briefcase is Cool.".capitalize() 'Hello, world. briefcase is cool.'
... well that sucks. '.'.join(t.capitalize() for t in text.split("."))
, anyone? 🤣
(seriously - upper()
on the first character is definitely preferable).
Easy! I'll work on that tomorrow :slightly_smiling_face:
@freakboy3742 I'm a bit stuck after revisiting this today.
Looking at the code, I think TemplateUnsupportedVersion
is only used when the branch does not exist. Is that what you're getting at with respect to changing it to a new InvalidTemplateBranch
exception? Basically: to clarify that it is not an "unsupported" branch, insofar as it does not even exist—it's just an invalid input?
Would the change then be to rename the exception to InvalidTemplateBranch
, and update the message to something like {branch} did not match any branches in the template repository {repo}.
?
Edit: I've pushed a commit with that change, at least, in case that's the extent of what you had in mind, but let me know either way :slightly_smiling_face:
A slight complication... the branch input is treated as any generic git ref, rather than explicitly a branch. It could be a tag or commit too. Does the "branch" terminology make sense, in that case? The CLI argument uses "branch", but could that name be reconsidered? With respect to the error message, I'm not sure how accurately such an error message needs to reflect the real usage of the argument, at least not if the real usage differs in scope from the name. It would be confusing for a user to pass a commit SHA and then be told it didn't match any branches. The user might interpret that as briefcase requiring specifically a branch and only a branch, rather than any git ref[^git-output]. On the other hand, I might just be over-thinking this bit.
[^git-output]: And, for what it's worth, git's own message is laughably obtuse in this situation, so not much help as guidance. git checkout not-a-branch
results in error: pathspec 'not-a-branch' did not match any file(s) known to git
.
(This bit, whether to call it a branch or not, also feels definitely outside the scope of this particular issue, even if it potentially muddies the communication still; clarifying it would probably be better suited for at least a separate PR, if not an entirely separate issue?)
Looking at the code, I think
TemplateUnsupportedVersion
is only used when the branch does not exist. Is that what you're getting at with respect to changing it to a newInvalidTemplateBranch
exception? Basically: to clarify that it is not an "unsupported" branch, insofar as it does not even exist—it's just an invalid input?Would the change then be to rename the exception to
InvalidTemplateBranch
, and update the message to something like{branch} did not match any branches in the template repository {repo}.
?
Didn't see this comment until after reviewing the code - but yes - that was exactly what I had in mind. Your analysis is dead on - the primary usage case for branches is supporting different Briefcase versions, which is why almost all the usage is tied to "template version". However, you can specify any branch name you want, which is where the current exception text falls apart.
A slight complication... the branch input is treated as any generic git ref, rather than explicitly a branch. It could be a tag or commit too. Does the "branch" terminology make sense, in that case? The CLI argument uses "branch", but could that name be reconsidered? ... On the other hand, I might just be over-thinking this bit.
I think that's probably overthinking it 😄 You're correct of course - but the set of people who work out you can use a tag or hash in place of a branch name aren't going to get confused if an error message refers to "branch
- And, for what it's worth, git's own message is laughably obtuse in this situation, so not much help as guidance.
Yeah - "don't follow git's UX" is good advice in almost any scenario... 🤣
Fixes #1118.
When cloning goes wrong, git usually provides a useful message. Examples of these are listed in the linked issue, and as far as I can tell, they always include a
fatal:
before the message details. Git sometimes also outputs information before thefatal:
sigil, but the exact content of that message varies, and appears to do so based on the server responding. From what I've seen, doesn't offer more information than Briefcase's "Unable to clone ..." message. For example, GitHub and Sourcehut say almost, but not quite the same things when you try to clone a repository that doesn't exist.ERROR: Repository not found.
with only a single trailing newlineRepository not found.
with two trailing newlines and noERROR:
preambleThat message is then followed by identical
fatal: ...
messages. Git already introduces enough variability in the formatting of itsfatal
messages (see the normalisation part of this PR), so trying to account for the variations introduced by any given git server would be futile, and with dubious value. So far as I can tell, git'sfatal
message provides the context that would be useful to a user, so that's what I've chosen to surface to the Briefcase user.The messages used in the test cases are based on the handful of real messages discussed in the issue, but have manufactured aspects documented in the
pytest.param
id
, meant to test the particular cases of the hint normalisation. I have not tried to find every error git can emit when cloning, and have not verified that any of them don't include thefatal:
sigil. It could be that the fallback hint will never be used in real life.PR Checklist: