push: improve 'no remote specified' hint #3213
Conversation
dvc/data_cloud.py
Outdated
| return self._init_remote(remote) | ||
|
|
||
| raise NoRemoteError(command) | ||
| has_any_remote = any(key.startswith("remote ") for key in self._config) |
There was a problem hiding this comment.
This feels as a part of the logic inside exception itself. So everything should be either inside or outside.
Also, key.startswith("remote ") is a bit fishy, we check for regex in other places.
There was a problem hiding this comment.
Got it, changed that to use list_options as I've found elsewhere. Remote URLs are required so finding any means that there's a valid remote that could be used.
dvc/config.py
Outdated
| "or use:\n" | ||
| " dvc {} -r <name>\n".format(command) | ||
| ) | ||
| def __init__(self, command, *, config=None): |
There was a problem hiding this comment.
It's to make sure config can only be used as a keyword argument.
There was a problem hiding this comment.
The reasoning for it is to prevent the exception from being initialized with some context that might not be a Config.
There was a problem hiding this comment.
So it is to make sure that we will never use it as a keyword argument? Isn't it kind of protecting the code from the developer?
There was a problem hiding this comment.
Exactly.
Edit: No, actually it's to make sure we always use it as a keyword argument.
There was a problem hiding this comment.
Nothing makes it special really. I just thought somebody might make a mistake and pass something else as an argument here. But I'll change it back.
There was a problem hiding this comment.
@fabiosantoscode this is a good point, and we actually have seen few times when wrong arguments are passed due to ordering, but I guess that there is no point in introducing that in this single place if we do differently everywhere else. We just need to be careful when using kwargs as args.
There was a problem hiding this comment.
I am +1 on using keyword only args in situations like this.
There was a problem hiding this comment.
@Suor So maybe we should enforce this rule throughout the code?
|
Approved it. A general consideration regarding messages like this - we've discussed adding more or less uniform HINTs in such situations, which will take into account context better, e.g. remote(s) name, having several remotes, command/options/api call. How this should be implemented and how far should we go is still TBD in #2559. |
|
Thx, @fabiosantoscode. |
dvc/config.py
Outdated
| has_any_remote = False | ||
| if config: | ||
| has_any_remote = bool( | ||
| config.list_options( | ||
| Config.SECTION_REMOTE_REGEX, Config.SECTION_REMOTE_URL | ||
| ) | ||
| ) | ||
|
|
||
| if has_any_remote: | ||
| msg = ( | ||
| "no remote specified. Setup default remote with\n" | ||
| " dvc remote default <remote name>\n" | ||
| "or use:\n" | ||
| " dvc {} -r <remote name>".format(command) | ||
| ) | ||
| else: | ||
| msg = ( | ||
| "no remote specified. Create a remote with\n" | ||
| " dvc remote add <remote name> <remote url>\n" | ||
| "To create a default remote use the -d flag:\n" | ||
| " dvc remote add -d <remote name> <remote url>" | ||
| ) |
There was a problem hiding this comment.
Having such a complex logic in the exception is self is rather odd. How about we process config in the code itself and then raise NoRemoteError with the pre-crafted message?
There was a problem hiding this comment.
Works for me. The message was being generated in __init__ so I kept it there.
dvc/config.py
Outdated
| "no remote specified. Create a remote with\n" | ||
| " dvc remote add <remote name> <remote url>\n" |
There was a problem hiding this comment.
This part is no relevan without -d, because if someone would've provided the remove, we would go if remote branch of execution (see below where you raise). So the hint here should assume -d.
There was a problem hiding this comment.
That's right!
|
@efiop I've tried to address your comments with a new commit. I've removed the |
4 participants
This tries to clear up the error message of NoRemoteError in case the user doesn't have any remotes, and provides instructions on how to create a remote, or a default remote. It also clears the extra newline at the end.
Fixes #3121
❗ 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. 🙏