Improve LockError message
#2765
Conversation
dvc/lock.py
Outdated
| @@ -15,6 +16,12 @@ | |||
|
|
|||
| DEFAULT_TIMEOUT = 5 | |||
|
|
|||
| ERROR_MESSAGE = ( | |||
| "cannot perform the cmd since DVC is busy and locked. You can delete " | |||
| ".dvc/lock to fix it, but first please make sure there's no other dvc " | |||
There was a problem hiding this comment.
Use upper case name (DVC vs dvc)
There was a problem hiding this comment.
Hmmm. He uses "DVC" first, but the 2nd time I think he's referring to the dvc process. Should wrap it in back ticks ` though.
There was a problem hiding this comment.
I would rephrase it. The but-clause makes it harder to read than needed. Also "busy and locked" is too abstract, what does it mean? And cmd -> command.
So, it's like:
Cannot perform the command since other dvc process is running for this repo. If you are sure this is not the case, you might remove .dvc/lock to fix this ...
Then command line hints go. However, ps xaf | grep dvc doesn't make sense for Windows, unless again they don't use bash or somehting there.
There was a problem hiding this comment.
👍 , I agreed wtih @Suor regarding rephrasing. Also, including command line hints to check running processes I think is way too much. Let's just remove this part completely.
dvc/lock.py
Outdated
| @@ -15,6 +16,12 @@ | |||
|
|
|||
| DEFAULT_TIMEOUT = 5 | |||
|
|
|||
| ERROR_MESSAGE = ( | |||
| "cannot perform the cmd since DVC is busy and locked. You can delete " | |||
| ".dvc/lock to fix it, but first please make sure there's no other dvc " | |||
There was a problem hiding this comment.
I would rephrase it. The but-clause makes it harder to read than needed. Also "busy and locked" is too abstract, what does it mean? And cmd -> command.
So, it's like:
Cannot perform the command since other dvc process is running for this repo. If you are sure this is not the case, you might remove .dvc/lock to fix this ...
Then command line hints go. However, ps xaf | grep dvc doesn't make sense for Windows, unless again they don't use bash or somehting there.
| @@ -48,9 +47,6 @@ def main(argv=None): | |||
|
|
|||
| cmd = args.func(args) | |||
| ret = cmd.run() | |||
| except LockError: | |||
| logger.exception("failed to lock before running a command") | |||
| ret = 250 | |||
There was a problem hiding this comment.
If this is about an error message only then why do we change the return code?
There was a problem hiding this comment.
@efiop might be able to answer this.
There was a problem hiding this comment.
@Suor What will be the command on cmd on Windows? tasklist | findstr dvc?
There was a problem hiding this comment.
LockError is based on DvcException and doesn't make it up here, as all CLI commands have try except DvcException in them. So we can remove this bit.
Updated message
|
There was a problem hiding this comment.
@algomaster99 looks good to me!
I have though some concerns (probably it's better to address them to @jorgeorpinel, @efiop ) about back-ticks . I think it's not a common practice to use markdown markup in UI.
|
@jorgeorpinel Capitalized dvc (DVC) shouldn't be enclosed in back-ticks, right? |
|
No back ticks, thanks! |
❗ 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 addresses. 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 #2520
Changed error message