-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
#1618 Custom message when cache and remote are in sync #4694
Conversation
@@ -11,6 +11,7 @@ class CmdDataStatus(CmdDataBase): | |||
STATUS_LEN = 20 | |||
STATUS_INDENT = "\t" | |||
UP_TO_DATE_MSG = "Data and pipelines are up to date." | |||
IN_SYNC_MSG = "Cache and remote '{remote}' are in sync." |
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.
There could be a better phrased message, I'm open to suggestions.
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.
Hm, Cache and remote are in sync
sounds a bit like cache and remote have the same list of cache files, which is actually not true, because we care about the cache for the local workspace or branches/tags (--all-tags, --all-branches). But this is still a good start 👍
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 used this paragraph from dvc status
documentation as a guide for what is happening and how it should be phrased:
For new and deleted data, the cache is different from remote storage. Bringing the two into sync requires dvc pull or dvc push. For the typical process to update the workspace, see Sharing Data And Model Files.
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.
So if you don't need to do anything to bring them into sync I thought we can call the state as "are in sync" :)
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'm keen to work out the best message cause this PR is all about making the message more accurate. Having an OK message kind of defeats the purpose...
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.
@tenitski Thanks for the attention to details! 🙏
@jorgeorpinel could you take a look, please?
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.
Minor: based on the elif
order below wouldn't it be better to put this new constant before UP_TO_DATE_MSG
?
As for the text itself, it seems good but mentioning the cache seems too low-level. Maybe "This repo is in sync with the remote." ? Also, may be no need to specify which remote since the user should know that already... But I guess for when you have a default remote, to make sure you're aware of which one was checked, appending the remote name at the end is not a bad idea.
One more thing, in the PR header there was a checkbox for the docs. This PR will require doc change, so could you please at least create an issue for it on https://github.com/iterative/dvc.org/issues ? |
When I created the PR I checked docs page for |
@tenitski Oh, good to know 👌 Pretty much https://dvc.org/doc/command-reference/status will need some adjustment in |
Oh, yeah, makes sense. I didn't think about other pages which may refer to this message. |
Addresses #1618 issue
Update
dvc status -r X
ordvc status -c
commands by printing out a more specific message if cache and remote are in sync with each other.