Change Using to implement the loan pattern #7468
Conversation
|
Round 3, I think. I'm pretty happy with this one. |
| } | ||
| } | ||
|
|
||
| object Manager { |
There was a problem hiding this comment.
my thought was to save Managed for an apply method taking an implicit function, once those arrive, which I think makes syntax a bit neater; at that point, this apply method on Manager could be deprecated
|
is there someone(s) who's looked at past iterations of this who could also review this version? |
Ichoran
left a comment
Ichoran
left a comment
There was a problem hiding this comment.
I like the usage pattern now!
There are some issues (one probably serious--equality test that is mean to be assignment, most minor) but I think it's a good design. I am unconvinced that the suppression thing is better than creating a new CompoundException that just lists everything in order, but I guess since it's already done it should be fine (might be useful).
| * when the manager is closed, regardless of any exceptions thrown | ||
| * during use. | ||
| * | ||
| * @note It is recommended for API designers to require an implicit `Manager` |
There was a problem hiding this comment.
I don't think this note is going to be enough for most people to understand what to do. You really need an example, or you probably shouldn't bother with the note.
There was a problem hiding this comment.
what do you think of the example I added?
There was a problem hiding this comment.
The example makes it clearer
|
@Ichoran the problem with a |
|
by the way, for someone merging: the first commit passed, but de-synced. |
|
@lrytz could you review the new docs? |
|
@NthPortal docs look good to me |
|
Speaking of the docs, I think it would be better to show an example in object |
NthPortal
force-pushed
the
topic/using-loan/PR
branch
from
78edc99 to
d16dd82
Compare
Use granular hierarchy for exception suppression in `Using.resource`.
NthPortal
force-pushed
the
topic/using-loan/PR
branch
from
cdbb577 to
667de59
Compare
|
@julienrf are the new documentation examples satisfactory? |
|
Yes, thanks! |
|
@Ichoran could you look over this again when you get the chance? I've fixed the major error and added better examples. |
Ichoran
left a comment
Ichoran
left a comment
There was a problem hiding this comment.
Looks great to me! I'd love to use this. It's similar to but considerably better than what I have in my personal library!
|
I think this is good to merge then |
|
If Julien, Rex and Nth are happy, I'm happy. |
|
(Note: scala/scala-dev#592 is still an open question) |
|
(that issue didn't generate a lot of discussion, which probably means that people are happy to have it. And anyway, I think this PR does improve the current API) |
|
@densh since this was partly inspired by your talk (https://www.youtube.com/watch?v=MV2eJkwarT4), perhaps you'd like to do a review pass? |
8 participants
Followup to #7415 and discussion there.
Based on top of #7457 - that can be reviewed an merged independently, or not.
Fixes scala/bug#11225.
Fixes scala/bug#11129.