exception handling

[casperdcl]

There are some instances where I think code is effectively unreachable (unless called in a different way). For example:

https://github.com/iterative/dvc/blob/5c76b46b460e00d3bb5c3f2ea87b19d961491f12/dvc/command/add.py#L17

means that

https://github.com/iterative/dvc/blob/5c76b46b460e00d3bb5c3f2ea87b19d961491f12/dvc/repo/add.py#L22

is never called. Also I feel they should both be raising RecursiveAddingWhileUsingFilename.

Also, the exception above is explicitly caught and the message and type completely suppressed, which seems odd:

https://github.com/iterative/dvc/blob/5c76b46b460e00d3bb5c3f2ea87b19d961491f12/dvc/command/add.py#L27-L29

Surely instead it's better to just let it be raised? If we want to suppress stack trace for user-friendliness, then six.raise_from instead?

[efiop]

@casperdcl logger.exception doesn't suppress the original exception. If you take a look at dvc/logger.py, you'll see that exception() is taking the last exception and prints it as error and adds the traceback if we are in verbose -v|--verbose mode. So usually the resulting error will be:

ERROR: failed to add file - can't use '--file' with multiple targets

[casperdcl]

@efiop ah ok. But maybe we should also print the exception type (KeyError etc) even when not verbose?

[efiop]

@casperdcl But then it will often contain "DvcException", which is not friendly and even might be scary for some users. We try to handle exceptions gracefully, so none of them get up to the user level in a raw form, but when they do manage to surface, then we ask for -v output and it shows us all the details that we need. I doesn't seem like exception class name is really needed in regular operation.

[casperdcl]

Closing as stale