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

data status: exclude remote status by default #9411

Merged
merged 1 commit into from
May 6, 2023
Merged

Conversation

dberenbaum
Copy link
Collaborator

Fixes #9410.

Needs a docs PR.

Before this PR:

$ dvc data status --help
usage: dvc data status [-h] [-q | -v] [--json] [--granular] [--unchanged]
                       [--untracked-files [{no,all}]] [--remote-refresh]

...

options:
  ...
  --remote-refresh      Refresh remote index.

After this PR:

$ dvc data status --help
usage: dvc data status [-h] [-q | -v] [--json] [--granular] [--unchanged]
                       [--untracked-files [{no,all}]] [--not-in-remote]
                       [--no-remote-refresh]

...

options:
  ...
  --not-in-remote       Show files not in remote.
  --no-remote-refresh   Use cached remote index (don't check remote).

@dberenbaum dberenbaum requested a review from efiop May 5, 2023 16:05
@@ -402,15 +402,23 @@ def test_missing_remote_cache(M, tmp_dir, dvc, scm, local_remote):
tmp_dir.dvc_gen({"dir": {"foo": "foo", "bar": "bar"}})
tmp_dir.dvc_gen("foobar", "foobar")

assert dvc.data_status(untracked_files="all") == {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@efiop Do we have a way to test remote refresh?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really and I don't think we need to.

@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (b044bcc) 91.61% compared to head (f8b9e66) 91.60%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9411      +/-   ##
==========================================
- Coverage   91.61%   91.60%   -0.01%     
==========================================
  Files         487      487              
  Lines       37778    37780       +2     
  Branches     5436     5436              
==========================================
  Hits        34610    34610              
- Misses       2612     2614       +2     
  Partials      556      556              
Impacted Files Coverage Δ
dvc/repo/data.py 98.31% <ø> (-1.69%) ⬇️
tests/unit/command/test_data_status.py 100.00% <ø> (ø)
dvc/commands/data.py 98.40% <100.00%> (+0.01%) ⬆️
tests/func/test_data_status.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@efiop
Copy link
Contributor

efiop commented May 6, 2023

For the record: some questions in #9410 (comment)

help="Show files not in remote.",
)
data_status_parser.add_argument(
"--no-remote-refresh",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@efiop I think refreshing is a more reasonable default because it is probably more intuitive and expected unless users are thinking deeply about how it compares to git. However, if you have a reason why you strongly prefer to rely on the index by default, I'm open to changing this back as long as we also keep the --not-in-remote option.

Copy link
Member

Choose a reason for hiding this comment

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

If it's an explicit flag, fetching latest changes make sense to me. I'm okay with showing a hint/summary to dvc push if needed by default.

@efiop efiop merged commit 2db07b0 into main May 6, 2023
@efiop efiop deleted the data-status-remote branch May 6, 2023 15:21
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.

data status: do not show "not in remote" status by default
3 participants