Add information to ambiguous error message. #5172
Conversation
Users could confuse the previous error message with a shell path issue, as though it could not resolve the installation of git-lfs. The proposed message in this commit offers additional insight by: - Detailing that LFS has not been installed for this repository. - Offering a fix (git lfs install) for the error. Relate issue #3048. Judging by the continued addition of upvotes, even after the issue has been closed, users are still running into this problem.
bk2204
left a comment
bk2204
left a comment
There was a problem hiding this comment.
Hey, thanks for the patch, and welcome to Git LFS! I think this is clearly a helpful improvement, and I had just one suggestion to make it a little easier to read.
commands/command_pull.go
Outdated
|
|
||
| if singleCheckout.Skip() { | ||
| fmt.Println(tr.Tr.Get("Skipping object checkout, Git LFS is not installed.")) | ||
| fmt.Println(tr.Tr.Get("Skipping object checkout, Git LFS is not installed for this repository. Consider installing it with 'git lfs install'.")) |
There was a problem hiding this comment.
I think this text is clearly an improvement, but I think it's longer than 80 characters, which might make it a bit difficult to read in some cases. Could we insert a newline here after repository. to make it easier to read on systems with small screens?
There was a problem hiding this comment.
I totally agree. I amended the PR with a multiline message. This makes it easier to read for both developers and end-users. However, I couldn't find an example of a multiline string in the project. If you prefer backticks, I can adjust it to that as well. Thank you for the warm welcome.
Increases line visibility for both programmers and end-users.
commands/command_pull.go
Outdated
| if singleCheckout.Skip() { | ||
| fmt.Println(tr.Tr.Get("Skipping object checkout, Git LFS is not installed.")) | ||
| fmt.Println(tr.Tr.Get("Skipping object checkout, Git LFS is not installed for this repository.\n" + | ||
| "Consider installing it with 'git lfs install'.")) |
There was a problem hiding this comment.
I think one of these lines has trailing whitespace, which is causing CI to fail.
While you're making that change, I think it will be easier for the code which extracts strings for translation if we leave it as one long string on one line, just with a newline in the middle.
There was a problem hiding this comment.
Easy. I amended the snippet to just one line with the newline character. Sorry it took three reviews for a one-line string. :)
Relate discussion in PR #5172. Simplifies string extraction. Passing local tests with `make test`.
bk2204
left a comment
bk2204
left a comment
There was a problem hiding this comment.
This looks good. Thanks so much for the patch.
3 participants
Users could confuse the previous error message with a shell path issue, as though it could not resolve the installation of git-lfs. The proposed message in this commit offers additional insight by:
Relate issue #3048. Judging by the continued addition of upvotes, even after the issue has been closed, users are still running into this problem.
Passing local test through the use of
make test.