async-interop / event-loop

An event loop interface for interoperability in PHP.
MIT License
170 stars 9 forks source link

Strict types #117

Closed kelunik closed 7 years ago

kelunik commented 7 years ago

Yes, who ever thought it was a good idea to introduce yet another behavior that might be toggled... we have to deal with strict types in the spec. We have to define whether loop implementations MUST or MUST NOT use strict typing. They call a lot of callbacks and thus implementations shouldn't differ there. Unfortunately, the callbacks are subject to the caller file's declaration, so the loop driver one.

kelunik commented 7 years ago

See https://twitter.com/kelunik/status/811854681790185472 and https://gist.github.com/kelunik/51d3e8620e0b54ea2c158d94689f423a.

joshdifabio commented 7 years ago

Oh. Very good point.

Within event loop implementations I think strict typing makes a lot of sense as callbacks are generally only dealing with fixed types (e.g. string $watcherId, int $sigNo) and mismatches are likely to indicate bugs, while even the mixed $data args will be specified by the same code which provides the callback, so there shouldn't be any reason to rely on weak typing for those either.

bwoebi commented 7 years ago

I do not care here in the loop, but tend to side with Josh here.

trowski commented 7 years ago

I agree with @joshdifabio, strict-typing is the way to go in loop implementations. At least the callbacks here are more consistent and concise than with promises, where this issue also needs to be addressed. Type declarations in callbacks is a serious oversight in strict vs. weak typing. Probably a feature at this point rather than a bug…

kelunik commented 7 years ago

Promise and loop should definitely be the same.

joshdifabio commented 7 years ago

Promise and loop should definitely be the same.

You might be right. I personally feel more strongly that promises should use weak typing than I do that the loop should use strong typing. So I guess I'd rather both weak than both strong.

kelunik commented 7 years ago

One major point against going strict is that it's the only thing that would make us require PHP 7.0 in the promise spec currently.

kelunik commented 7 years ago

https://github.com/async-interop/promise/issues/21#issuecomment-269020883 means we're probably going with weak here, too.

joshdifabio commented 7 years ago

I don't think it'd be a problem to take a different approach between the two specs but I also don't see any issues with both being weak.

kelunik commented 7 years ago

@joshdifabio I think it's weird to be inconsistent there.

joshdifabio commented 7 years ago

Okay. I'm 50/50 on it so I'll keep quiet.

trowski commented 7 years ago

I still prefer strict types, but also understand we don't want to force that on everyone using these specs, so it looks like the logical choice is weak types…

kelunik commented 7 years ago

@trowski: The callable arguments here are anyway always only form the loop and should be fine in all cases, so it doesn't really matter here.

trowski commented 7 years ago

@kelunik Watcher callbacks may have type declarations for the $data parameter. This is far less of an issue than for Promise callbacks since the creator of the watcher provides the data, but there could be casting happening there with weak types.

bwoebi commented 7 years ago

Closing as #123 has been merged.

kelunik commented 7 years ago

Done via #123.