Skip to content
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

exception text: clarify error message #4339

Merged
merged 1 commit into from
Aug 13, 2020
Merged

Conversation

bobertlo
Copy link
Contributor

@bobertlo bobertlo commented Aug 5, 2020

The default error message is often invoked from running dvc pull and
in this case it is generally caused by not pushing to the remote. I
spent some time thinking about a replacement message and this seems to
be the most appropriate text I could think of.

Fixes #4316

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

dvc/exceptions.py Outdated Show resolved Hide resolved
@bobertlo
Copy link
Contributor Author

bobertlo commented Aug 5, 2020

I guess I will admit this does not "solve" the original issue. This error message always really bothered me though :)

@bobertlo
Copy link
Contributor Author

bobertlo commented Aug 7, 2020

I edited this to print a link to the troubleshooting page, edited in iterative/dvc.org#1671 with a short explanation of common problems.

Comment on lines +271 to +272
"Checkout failed for following targets:\n{}\nIs your "
"cache up to date?\n{}".format(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache question seems unclear, how about just:

Suggested change
"Checkout failed for following targets:\n{}\nIs your "
"cache up to date?\n{}".format(
"Checkout failed for following targets:\n{}\n"
"Visit {} for more info.".format(

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Visit {} for more info." sounds like holding a user hostage. We do need better exception message, which can be done by making _fetch and _checkout collaborate with each other, but making CheckoutError message generic is not a solution, it only adds confusion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skshetry Good point. Maybe for a lack of better message (because a more advanced approach is needed), let's just append "Vinit {} for more info"(or someting like that) to the old message? Just in the interest of time, since the proper-proper solution will require quite a bit more effort.

Copy link
Contributor Author

@bobertlo bobertlo Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efiop I was sort of thinking the same thing myself. I updated it to that and removed the "Fixes #4316" since that issue goes deeper than this. I think other changes are unrelated to this PR at this point?

Also I should note the link is live on dvc.org now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efiop, yeah, that sounds good. But let's keep the issue open.

@bobertlo
Copy link
Contributor Author

I suppose the errors provided during fetch are pretty clear. I mainly did not like the 'did you forget to fetch?' message. I can change this one I get near a terminal again.

@bobertlo
Copy link
Contributor Author

@efiop Okay, made that exact change. I agree that since we're linking to the troubleshooting page we don't need anything more than the above error messages in the context.

@bobertlo
Copy link
Contributor Author

bobertlo commented Aug 10, 2020 via email

Copy link
Member

@efiop efiop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bobertlo !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Did you forget to fetch? is generally not the solution to my problem
5 participants