Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rwlock: include actionable in error message #3270

Merged
merged 2 commits into from Feb 5, 2020
Merged

rwlock: include actionable in error message #3270

merged 2 commits into from Feb 5, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jan 31, 2020

Fix: #3164

@ghost ghost requested review from efiop and shcheklein January 31, 2020 22:14
dvc/rwlock.py Outdated Show resolved Hide resolved
Copy link
Member

@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.

please, check some comments

dvc/rwlock.py Outdated Show resolved Hide resolved
Suor
Suor previously requested changes Feb 3, 2020
Copy link
Contributor

@Suor Suor left a comment

Choose a reason for hiding this comment

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

Some dedup is needed. We also have similar message in dvc.lock.

@ghost
Copy link
Author

ghost commented Feb 4, 2020

I will handle the duplication in another pull request, as agreed on planning.
I think both methods can be abstracted.

@ghost
Copy link
Author

ghost commented Feb 5, 2020

@efiop , I already had the patch for it, since this PR isn't closed yet, I will upload it here.

@ghost ghost requested review from shcheklein and Suor February 5, 2020 00:14
@ghost ghost dismissed stale reviews from Suor and shcheklein February 5, 2020 00:14

reduce duplication

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks!

@efiop efiop merged commit d9ed98b into iterative:master Feb 5, 2020
@ghost ghost deleted the fix-3164 branch February 5, 2020 14:53
Comment on lines +65 to +70
blockers = [
info
for path, infos in lock[mode].items()
if path_info.overlaps(path)
if info not in (infos if type(infos) is list else [infos])
]
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is broken now. We create a list of info passed as param repeated 0 to several times, while in previous logic we were listing infos we've got from lock.

The right logic I guess is:

blockers = [
    blocker
    for path, infos in lock[mode].items()
    if path_info.overlaps(path)
    for blocker in (infos if isinstance(infos, list) else [infos])
    if blocker != info
]

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in #3285.

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.

improve message for rwlock error
3 participants