add: meaningful message upon adding overlapping paths#3296
Conversation
6a8798f to
c352bec
Compare
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 🙂
c352bec to
580786a
Compare
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
There was a problem hiding this comment.
| except OverlappingOutputPathsError as e: | |
| except OverlappingOutputPathsError as exc: |
DS and others complain about such naming.
There was a problem hiding this comment.
Also please create a new line after \n here.
|
@pared Also check the tests, please. |
9a1d0fa to
3adcbd2
Compare
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).
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