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

Do something like withManager #137

Closed
ndmitchell opened this Issue Jul 21, 2015 · 6 comments

Comments

Projects
None yet
2 participants
@ndmitchell
Contributor

ndmitchell commented Jul 21, 2015

It seems a little weird that you have a creation function (newManager) and a closing function (closeManager), but no function that lets me do something using the with pattern. I'm not sure I get why withManager has to die? Is closeManager no longer necessary? If not, withManager seems like it has value, even if @merijn might not want to be using it in his example.

From #125 originally.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jul 21, 2015

Owner

That's a good point: we should deprecate closeManager too. The point is that the lifetime of the Manager is handled automatically, and manually closing things out is not necessary. This is similar to how in resource-pool, you can create a pool but not destroy one.

Owner

snoyberg commented Jul 21, 2015

That's a good point: we should deprecate closeManager too. The point is that the lifetime of the Manager is handled automatically, and manually closing things out is not necessary. This is similar to how in resource-pool, you can create a pool but not destroy one.

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell Jul 21, 2015

Contributor

That's fine then - I suggest doing it immediately and making clear in the docs. Perhaps rewrite withManager to not call closeManager, since one way of fixing deprecated warnings is to inline the function that is deprecated and refactor from there (that's what I always do).

Contributor

ndmitchell commented Jul 21, 2015

That's fine then - I suggest doing it immediately and making clear in the docs. Perhaps rewrite withManager to not call closeManager, since one way of fixing deprecated warnings is to inline the function that is deprecated and refactor from there (that's what I always do).

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jul 21, 2015

Owner

Agreed, that makes sense. Mind looking at 32b02a7 before I release?

Owner

snoyberg commented Jul 21, 2015

Agreed, that makes sense. Mind looking at 32b02a7 before I release?

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell Jul 21, 2015

Contributor

I've made a bunch of line notes.

Contributor

ndmitchell commented Jul 21, 2015

I've made a bunch of line notes.

@ndmitchell

This comment has been minimized.

Show comment
Hide comment
@ndmitchell

ndmitchell Jul 21, 2015

Contributor

All looks good now. Still conceptually weird to see you suggesting people rely on the GC for resource finalisation :)

Contributor

ndmitchell commented Jul 21, 2015

All looks good now. Still conceptually weird to see you suggesting people rely on the GC for resource finalisation :)

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jul 21, 2015

Owner

True, it is quite different from what I typically recommend. If you look at the internals of how this all works, it will make a lot more sense: the manager puts itself to sleep when unused, wakes up when used again, and when all references disappear, is garbage collected. You can manually kill it early, but that's more likely to trigger a bug than anything else.

Alright, releasing the new version.

Owner

snoyberg commented Jul 21, 2015

True, it is quite different from what I typically recommend. If you look at the internals of how this all works, it will make a lot more sense: the manager puts itself to sleep when unused, wakes up when used again, and when all references disappear, is garbage collected. You can manually kill it early, but that's more likely to trigger a bug than anything else.

Alright, releasing the new version.

@snoyberg snoyberg closed this Jul 21, 2015

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