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

Use a simple get/set accessor instead #149

Closed
wants to merge 1 commit into from
Closed

Use a simple get/set accessor instead #149

wants to merge 1 commit into from

Conversation

bwoebi
Copy link
Member

@bwoebi bwoebi commented Jan 31, 2017

As we ended up disagreeing on a common global accessor, I propose simplifying the whole accessor to just a get()+set() method pair.

This ensures that the loop will be interoperable at all (otherwise we'd end up having different loops active or worse things).

Now, every library can choose its own accessors as desired.
This should resolve the current point of contention.

Is this something we all can agree on?
\cc @kelunik @trowski @clue @WyriHaximus @martinschroeder @jsor @cboden @davidwdan @mbonneau @rdlowrey (please leave a thumbs up or down)

After that, I guess we can relatively quickly go to tagging v1 then as I haven't seen much objection on the Driver interface itself at all.


/**
* Accessor to allow global access to the event loop.
* Accessor to allow global access to the event loop
Copy link
Member

Choose a reason for hiding this comment

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

Missing dot.

src/Loop.php Outdated

/**
* Retrieve the event loop driver that is in scope.
* Retrieve the current event loop driver
*
* @return Driver
Copy link
Member

Choose a reason for hiding this comment

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

Must be @return Driver|null then.

Copy link
Member Author

Choose a reason for hiding this comment

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

right, will fix.

@WyriHaximus
Copy link
Member

Looks good 👍

Regarding driver specs and feedback I'll ask @clue and @cboden to respond with feedback or give me a list (if any).

@bwoebi bwoebi mentioned this pull request Feb 1, 2017
@bwoebi
Copy link
Member Author

bwoebi commented Feb 2, 2017

@WyriHaximus There seems to be general approval to this PR…

So, looking forward to the feedback from @clue and @cboden, then I'll merge this if we can agree on everything (also Driver).

@cboden
Copy link

cboden commented Feb 2, 2017

I have a busy week at work. I'll take a look as soon as I can which probably won't be until the weekend.

@bwoebi
Copy link
Member Author

bwoebi commented Feb 7, 2017

A quick reminder @cboden :-)

@kelunik
Copy link
Member

kelunik commented Apr 17, 2017

Closing, as project discontinued for now.

@kelunik kelunik closed this Apr 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants