Navigation Menu

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

Jump to bottom

Change Using to implement the loan pattern #7468

Merged
merged 2 commits into from Dec 26, 2018
Merged

Change Using to implement the loan pattern #7468

merged 2 commits into from Dec 26, 2018

Conversation

NthPortal
Copy link
Contributor

@NthPortal NthPortal commented Nov 27, 2018 •
edited

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.

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Nov 27, 2018
@NthPortal NthPortal mentioned this pull request Nov 27, 2018
Closed
@NthPortal
Copy link
Contributor Author

Round 3, I think. I'm pretty happy with this one.

NthPortal
NthPortal commented Nov 28, 2018
View reviewed changes
src/library/scala/util/Using.scala
}
}

object Manager {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@SethTisue
Copy link
Member

is there someone(s) who's looked at past iterations of this who could also review this version?

@NthPortal
Copy link
Contributor Author

@dwijnand or @julienrf perhaps?

Ichoran
Ichoran suggested changes Dec 6, 2018
View reviewed changes
Copy link
Contributor

@Ichoran Ichoran left a comment

Choose a reason for hiding this comment

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

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

src/library/scala/util/Using.scala
* when the manager is closed, regardless of any exceptions thrown
* during use.
*
* @note It is recommended for API designers to require an implicit `Manager`
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you think of the example I added?

Copy link
Contributor

Choose a reason for hiding this comment

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

The example makes it clearer

src/library/scala/util/Using.scala Outdated Show resolved Hide resolved
src/library/scala/util/Using.scala Outdated Show resolved Hide resolved
src/library/scala/util/Using.scala Outdated Show resolved Hide resolved
src/library/scala/util/Using.scala Outdated Show resolved Hide resolved
test/junit/scala/util/UsingTest.scala Outdated Show resolved Hide resolved
@NthPortal
Copy link
Contributor Author

@Ichoran the problem with a CompoundException is you lose the severity and meaning of the original exception, unless you specifically remember to unwrap it. This is particularly problematic for a VirtualMachineError, which must be propagated so that other code knows to panic and abort.

@NthPortal
Copy link
Contributor Author

by the way, for someone merging: the first commit passed, but de-synced. /sync will briefly fix it, but not permanently for some reason

@NthPortal
Copy link
Contributor Author

@lrytz could you review the new docs?

@julienrf julienrf mentioned this pull request Dec 10, 2018
Closed
@lrytz
Copy link
Member

lrytz commented Dec 10, 2018

@NthPortal docs look good to me

@julienrf
Copy link
Contributor

Speaking of the docs, I think it would be better to show an example in object Using of how to access from files. The current example shows a hypothetical resource1 but it would be nice to have a full example of how to read a file. (Just reading at the full docs I’m not sure how to use Using to read a file)

@NthPortal
Copy link
Contributor Author

@julienrf are the new documentation examples satisfactory?

@julienrf
Copy link
Contributor

Yes, thanks!

@NthPortal
Copy link
Contributor Author

@Ichoran could you look over this again when you get the chance? I've fixed the major error and added better examples.

Ichoran
Ichoran approved these changes Dec 22, 2018
View reviewed changes
Copy link
Contributor

@Ichoran Ichoran 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 to me! I'd love to use this. It's similar to but considerably better than what I have in my personal library!

@NthPortal
Copy link
Contributor Author

I think this is good to merge then

@dwijnand
Copy link
Member

If Julien, Rex and Nth are happy, I'm happy.

@dwijnand dwijnand merged commit 3a76f41 into scala:2.13.x Dec 26, 2018
@NthPortal NthPortal deleted the topic/using-loan/PR branch December 26, 2018 22:43
@NthPortal
Copy link
Contributor Author

(Note: scala/scala-dev#592 is still an open question)

@julienrf
Copy link
Contributor

(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)

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Jan 8, 2019
@SethTisue
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julienrf julienrf julienrf left review comments

@som-snytt som-snytt som-snytt left review comments

@Ichoran Ichoran Ichoran approved these changes

Assignees
No one assigned
Labels
release-notes worth highlighting in next release notes
Projects
None yet
Milestone
2.13.0-RC1
8 participants
@NthPortal @SethTisue @lrytz @julienrf @dwijnand @som-snytt @Ichoran @scala-jenkins