{"action":"closed","issue":{"url":"https://api.github.com/repos/amphp/amp/issues/56","repository_url":"https://api.github.com/repos/amphp/amp","labels_url":"https://api.github.com/repos/amphp/amp/issues/56/labels{/name}","comments_url":"https://api.github.com/repos/amphp/amp/issues/56/comments","events_url":"https://api.github.com/repos/amphp/amp/issues/56/events","html_url":"https://github.com/amphp/amp/issues/56","id":194843423,"number":56,"title":"Version 2 Review","user":{"login":"kelunik","id":2743004,"avatar_url":"https://avatars3.githubusercontent.com/u/2743004?v=3","gravatar_id":"","url":"https://api.github.com/users/kelunik","html_url":"https://github.com/kelunik","followers_url":"https://api.github.com/users/kelunik/followers","following_url":"https://api.github.com/users/kelunik/following{/other_user}","gists_url":"https://api.github.com/users/kelunik/gists{/gist_id}","starred_url":"https://api.github.com/users/kelunik/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kelunik/subscriptions","organizations_url":"https://api.github.com/users/kelunik/orgs","repos_url":"https://api.github.com/users/kelunik/repos","events_url":"https://api.github.com/users/kelunik/events{/privacy}","received_events_url":"https://api.github.com/users/kelunik/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":25,"created_at":"2016-12-11T17:17:05Z","updated_at":"2017-03-12T17:40:28Z","closed_at":"2017-03-12T17:40:28Z","body":"@trowski @bwoebi @rdlowrey I reviewed Amp v2.0 and here are my comments.\r\n\r\n`Internal\\WhenQueue`\r\n====================\r\n\r\nHow important is the layer of indirection? If we're worried about the call stack, should we instead resolve promises asynchronously?\r\n\r\nIt could also be renamed to a more generic name like `CallbackQueue` or `CallQueue`.\r\n\r\n`Internal\\Placeholder`\r\n======================\r\n\r\nAnnotation for `$onResolved` is `callable|\\Amp\\Internal\\WhenQueue|null`, should be `?WhenQueue` or ?`WhenQueue|null` instead.\r\n\r\n`Coroutine`\r\n===========\r\n\r\nWhy does it not accept a callable as constructor parameter in addition of generators?\r\n\r\n`Emitter`\r\n=========\r\n\r\n```php\r\nif ($this->resolved) {\r\n    return;\r\n}\r\n```\r\n\r\n^ Does that ignore double resolution silently?\r\n\r\n`Pause`\r\n=======\r\n\r\nDo we really need $value there? What about `keep_alive = false`?\r\n\r\n`Internal\\LazyPromise`\r\n======================\r\n\r\nWhy is this one internal? Why is Amp\\lazy suggested to be used only? Why do we even have `Amp\\lazy`? Instead of using arguments to Amp\\lazy, those parameters can just be use'd in a closure by the user instead of providing them as arguments.\r\n\r\n`Amp\\wrap` vs. `Amp\\coroutine`\r\n============================\r\n\r\nIt is weird to have both and usage will be confusion for users.\r\n\r\n`Amp\\timeout`\r\n=============\r\n\r\nI think `Amp\\timeout` shouldn't keep the loop running. The watchers should probably be unreferenced.\r\n\r\n`Amp\\lift`\r\n==========\r\n\r\nDo we need `lift`? I always ask that when reading the docs regarding it.\r\n\r\n`Amp\\filter`\r\n============\r\n\r\n`Amp\\filter` filters observables now instead of promises."}}
{"actor":{"display_login":"kelunik"}}