add: meaningful message upon adding overlapping paths #3296
Conversation
pared
force-pushed
the
3218_add_in_dir
branch
from
6a8798f to
c352bec
Compare
|
Before: After: |
dvc/exceptions.py
Outdated
| class AddOverlappingPathsError(DvcException): | ||
| def __init__(self, parent, overlapping_out): | ||
| super().__init__( | ||
| "Cannot add '{out}', because it is overlapping with other DVC " | ||
| "tracked output: '{parent}'.\nTo include '{out}' in '{parent}', " | ||
| "run 'dvc commit {parent_stage}'".format( | ||
| out=overlapping_out, | ||
| parent=parent, | ||
| parent_stage=parent.stage.relpath, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Let's do it a bit differently. Let's only keep OverlappingOutputPathsError, but raise it with specific message instead of hardcoding it into the exception class itself. That way we won't spawn new entities and it will be nicely catchable from outside if needed 🙂
pared
force-pushed
the
3218_add_in_dir
branch
from
c352bec to
580786a
Compare
dvc/repo/add.py
Outdated
| except OverlappingOutputPathsError as e: | ||
| msg = ( | ||
| "Cannot add '{out}', because it is overlapping with other " | ||
| "DVC tracked output: '{parent}'.\nTo include '{out}' in " |
There was a problem hiding this comment.
It is better to move a line down if you have \n. E.g.
"Cannot add '{out}', because it is overlapping with other "
"DVC tracked output: '{parent}'.\n"
"To include '{out}' in ..."
much easier to comprehend that way. Or was it black who did that?
There was a problem hiding this comment.
nah, black gets in the way only when lines do not match, I probably did that to save some lines :P
dvc/repo/add.py
Outdated
| repo.check_modified_graph(stages) | ||
| try: | ||
| repo.check_modified_graph(stages) | ||
| except OverlappingOutputPathsError as e: |
There was a problem hiding this comment.
| except OverlappingOutputPathsError as e: | |
| except OverlappingOutputPathsError as exc: |
DS and others complain about such naming.
tests/func/test_add.py
Outdated
| dvc.add(os.path.join("dir", "file2")) | ||
| assert str(e.value) == ( | ||
| "Cannot add 'dir/file2', because it is overlapping with other DVC " | ||
| "tracked output: 'dir'.\nTo include 'dir/file2' in 'dir', run " |
There was a problem hiding this comment.
Also please create a new line after \n here.
|
@pared Also check the tests, please. |
pared
force-pushed
the
3218_add_in_dir
branch
2 times, most recently
from
9a1d0fa to
3adcbd2
Compare
dvc/repo/__init__.py
Outdated
| "Paths for outs:\n'{}'('{}')\n'{}'('{}')\noverlap." | ||
| " To avoid unpredictable behaviour, " |
There was a problem hiding this comment.
minor: Please use consistent formatting. You skip trailing space in the first line but add it in the second one. IIRC prefered way to have a trailing space rather than a leading space. CC @jorgeorpinel
There was a problem hiding this comment.
Agree about consistency. Idk what's preferred though, I used to put trailing space last but later I noticed most of you use trailing first so rn there's a mix throughout docs. Maybe trailing space first is best because it makes it extra obvious that it's a continuation string? Up to you, I just rec to be consistent (available to review all existing strings once you decide, please lmk).
pared
force-pushed
the
3218_add_in_dir
branch
from
3adcbd2 to
bc81d96
Compare
|
minor: we use sometimes outs sometimes outputs/output in this PR - would be probably good to use one of those consistently (probably output/outputs to avoid slang-slang) |
❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏
Fixes #3218