Skip to content

Conversation

@algomaster99
Copy link
Contributor

@algomaster99 algomaster99 commented Nov 8, 2019

  • ❗ 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

Screenshot from 2019-11-09 04-31-41

@algomaster99 algomaster99 requested review from a user, Suor and efiop November 8, 2019 23:02
@algomaster99
Copy link
Contributor Author

@efiop @Suor @MrOutis
Let's decide how the error message should be then I will fix the codeclimate issue by assigning the message to a variable. πŸ˜„

@efiop efiop requested review from jorgeorpinel and pared November 8, 2019 23:19
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better language suggestion:

@algomaster99 algomaster99 changed the title Improve lock error message Improve LockError message Nov 9, 2019
dvc/lock.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use upper case name (DVC vs dvc)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ‘ , 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.

Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One little change, and one thing to consider.
Also, do you, by any chance, have a way to trigger this message? Maybe some script that could be shown through ascinema?

EDIT: sorry, didn't notice the print screen

dvc/lock.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dvc/main.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is about an error message only then why do we change the return code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@efiop might be able to answer this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Suor What will be the command on cmd on Windows? tasklist | findstr dvc?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@algomaster99
Copy link
Contributor Author

Updated message

Screenshot from 2019-11-14 02-40-30

Copy link
Contributor

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 jorgeorpinel self-requested a review November 14, 2019 03:18
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please just update dvc -> DVC per #2765 (comment). Thanks

@algomaster99
Copy link
Contributor Author

@jorgeorpinel Capitalized dvc (DVC) shouldn't be enclosed in back-ticks, right?

@jorgeorpinel
Copy link
Contributor

No back ticks, thanks!

Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM πŸš€

@efiop efiop merged commit 6147b93 into treeverse:master Nov 14, 2019
@algomaster99 algomaster99 deleted the improve-lock-error-message branch November 14, 2019 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lock: improve error message when .dvc/lock is taken

6 participants