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

Add Loop::getHandle #121

Closed
wants to merge 1 commit into from
Closed

Add Loop::getHandle #121

wants to merge 1 commit into from

Conversation

kelunik
Copy link
Member

@kelunik kelunik commented Dec 23, 2016

Previously we advised to use Loop::get()->getHandle(), because it's a method only required by very few components, namely the filesystem libraries, which need the underlying handle to operate.

We argued that we get a smaller autocomplete list that way. I think consistency is more important.

Previously we advised to use Loop::get()->getHandle(), because it's a method only required by very few components, namely the filesystem libraries, which need the underlying handle to operate.

We argued that we get a smaller autocomplete list that way. I think consistency is more important.
@kelunik kelunik requested review from trowski and bwoebi December 24, 2016 16:36
@bwoebi bwoebi removed their request for review December 24, 2016 16:53
@trowski
Copy link
Contributor

trowski commented Dec 24, 2016

I still feels like it clutters the API that application writers would need to use. Only libs should be concerned about the specific loop implementation being used.

@bwoebi
Copy link
Member

bwoebi commented Dec 24, 2016

I agree with @trowski — the autocomplete list is not important. It's about people looking at the Loop class and finding (for them) irrelevant functions.

@kelunik
Copy link
Member Author

kelunik commented Dec 29, 2016

I'll close this one for now, we can always add it in a v1.1.

@kelunik kelunik closed this Dec 29, 2016
@kelunik kelunik deleted the loop-handle branch December 29, 2016 15:37
@kelunik
Copy link
Member Author

kelunik commented Jan 8, 2017

@trowski @bwoebi We might want to add it, because otherwise filesystem has to depend on event-loop instead of event-loop-api, see #129.

@joshdifabio
Copy link
Contributor

joshdifabio commented Jan 8, 2017

It won't if event-loop-api covers the API of both classes (bot not the SPI). I don't see any issues with that myself.

@kelunik
Copy link
Member Author

kelunik commented Jan 8, 2017

We said event-loop-api should cover everything except Driver, if we make it even more fine granular, I think nobody will get what's actually covered.

@joshdifabio
Copy link
Contributor

joshdifabio commented Jan 8, 2017

To me, the important thing is to be able to make changes which are breaking for loop implementors (e.g. add a method to Driver) without forcing clients to also change their version constraints. I.e. if you plan to implement or extend Driver then you must require event-loop, otherwise you should require event-loop-api.

I think the value is less in separating Loop/Driver and more in separating API & SPI.

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.

5 participants