-
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
Fix #6205: Fixed diff summary to only show states with values. #6229
Conversation
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.
@hadasarik thanks for the PR! See my comments, there's a couple small issues
dvc/command/diff.py
Outdated
states_values = [] | ||
for state in states: | ||
state_value = summary[state] | ||
if state_value > 0: | ||
states_values.append(f"{state_value} {state}") | ||
|
||
ui.write(fmt.format_map(summary)) | ||
ui.write("files summary: " + ", ".join(states_values)) |
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 personally find loops to be confusing for small stuff. Generators/Comprehensions might be clearer of the intent, eg:
summary = ", ".join(generator)
Also, ui.write
is basically a small wrapper around print
, so you could get rid of "+" and just do:
ui.write("files summary:", summary)
WDYT?
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.
@skshetry Fixed :)
Hi @hadasarik @skshetry @pmrowla This require small docs updates to https://dvc.org/doc/command-reference/diff and https://dvc.org/doc/command-reference/exp/apply please (find |
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Fixes: #6205
Thank you for the contribution - we'll try to review it as soon as possible. π