-
Notifications
You must be signed in to change notification settings - Fork 251
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
Deprecate lockfile.Locker.RecursiveLock (alternative to #1376) #1344
Conversation
dc69c92
to
9b4099f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not fix it?
I didn't think it entirely through but assume we still need it for the concurrent pull work, don't we?
Sure, in principle. I don’t immediately know how to do that
I don’t know, I haven’t looked into that. … and I can’t spend time on this for the next few weeks (months?) at least. In the meantime, I want the problem to be recorded and documented, to prevent accidents. We can always un-deprecate this after the implementation is fixed. |
|
Yes, that sounds like a good idea. LGTM, @nalind are you on board? |
It's fine, though the prospect of keeping an API around with known breakage is troubling. If it's only there for a WIP caller that hasn't been merged and isn't actively being worked on, I'm more inclined to just remove it outright and revisit it when it becomes relevant again. |
It is not valid as currently implemented, and there is no known user. Signed-off-by: Miloslav Trmač <[email protected]>
9b4099f
to
8748134
Compare
Sure: #1376 . I don’t have a strong preference. (I have updated this one to deprecate also the callers in C/I/L stores; they seem to be public but can’t actually be directly accessed by external users anyway.) |
Lets remove it. |
Remove lockfile.Locker.RecursiveLock (alternative to #1344)
It is not valid as currently implemented, and there is no known user.
Cc: @vrothberg : This was added in #347 , supposedly for the concurrent copy detection.