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 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.
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.
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
@@ -113,7 +113,7 @@ func pull(filter *filepathfilter.Filter) { | |||
} | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -113,7 +113,8 @@ func pull(filter *filepathfilter.Filter) { | |||
} | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`.
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 good. Thanks so much for the patch.
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
.