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

add: ignore duplicated targets? #6286

Merged
merged 1 commit into from Jul 12, 2021

Conversation

skshetry
Copy link
Member

@skshetry skshetry commented Jul 6, 2021

# before
$ dvc add foo foo
ERROR: output 'foo' is already specified in stages:

# now

Screen Shot 2021-07-07 at 12 41 34

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

@skshetry skshetry added enhancement Enhances DVC ui user interface / interaction labels Jul 6, 2021
@skshetry skshetry added this to the UI improvements milestone Jul 6, 2021
@skshetry skshetry requested a review from dberenbaum July 6, 2021 06:24
@skshetry skshetry self-assigned this Jul 6, 2021
@skshetry skshetry added this to In progress in DVC 29 June - 12 July 2021 via automation Jul 6, 2021
@skshetry skshetry requested a review from a team as a code owner July 6, 2021 06:24
@skshetry skshetry moved this from In progress to Review in progress in DVC 29 June - 12 July 2021 Jul 6, 2021
@skshetry skshetry changed the title add: ignore duplicated entries add: ignore duplicated targets? Jul 6, 2021
@dberenbaum
Copy link
Contributor

What about a warning? It's not clear to me whether this would ever be intended behavior, or whether it would be important for someone to know they had added the same target twice.

@skshetry
Copy link
Member Author

skshetry commented Jul 7, 2021

@dberenbaum, it could happen with typos when similar targets exist. The main reason for this PR is that our internals cannot handle this properly (especially the graph checks), so we have to handle it at the top. I'll add a warning then.

@skshetry skshetry force-pushed the duplicate-targets branch 2 times, most recently from 144dfeb to f4001e5 Compare July 7, 2021 06:56
@skshetry skshetry requested a review from efiop July 7, 2021 06:57
dupes = counter.most_common(n=2)
if dupes:
msg = ", ".join(f"[b]{key}[/]" for key, _ in dupes)
ui.error_write(f"ignoring duplicated targets: {msg}", styled=True)
Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, the capitalization of message (or, not) could be another topic for the UI discussion.

Copy link
Contributor

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

LGTM!

@skshetry skshetry merged commit beb610c into iterative:master Jul 12, 2021
@skshetry skshetry deleted the duplicate-targets branch July 12, 2021 14:58
DVC 29 June - 12 July 2021 automation moved this from Review in progress to Done Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhances DVC ui user interface / interaction
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants