{"action":"created","issue":{"url":"https://api.github.com/repos/keyboardio/Kaleidoscope/issues/318","repository_url":"https://api.github.com/repos/keyboardio/Kaleidoscope","labels_url":"https://api.github.com/repos/keyboardio/Kaleidoscope/issues/318/labels{/name}","comments_url":"https://api.github.com/repos/keyboardio/Kaleidoscope/issues/318/comments","events_url":"https://api.github.com/repos/keyboardio/Kaleidoscope/issues/318/events","html_url":"https://github.com/keyboardio/Kaleidoscope/pull/318","id":321993840,"number":318,"title":"Define all hooks and event handlers at one point","user":{"login":"noseglasses","id":29701465,"avatar_url":"https://avatars3.githubusercontent.com/u/29701465?v=4","gravatar_id":"","url":"https://api.github.com/users/noseglasses","html_url":"https://github.com/noseglasses","followers_url":"https://api.github.com/users/noseglasses/followers","following_url":"https://api.github.com/users/noseglasses/following{/other_user}","gists_url":"https://api.github.com/users/noseglasses/gists{/gist_id}","starred_url":"https://api.github.com/users/noseglasses/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/noseglasses/subscriptions","organizations_url":"https://api.github.com/users/noseglasses/orgs","repos_url":"https://api.github.com/users/noseglasses/repos","events_url":"https://api.github.com/users/noseglasses/events{/privacy}","received_events_url":"https://api.github.com/users/noseglasses/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2018-05-10T16:24:00Z","updated_at":"2018-05-10T19:51:36Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/keyboardio/Kaleidoscope/pulls/318","html_url":"https://github.com/keyboardio/Kaleidoscope/pull/318","diff_url":"https://github.com/keyboardio/Kaleidoscope/pull/318.diff","patch_url":"https://github.com/keyboardio/Kaleidoscope/pull/318.patch"},"body":"This commit adds an X Macro _FOR_EACH_EVENT_HANDLER\r\nto define all information\r\nabout hook functions and coresponding event handler methods\r\nat a single place.\r\nBy this means it is possible to prevent code redundancies\r\nand make it as simple as possible to add new hooks\r\nor to change their signature in future revisions.\r\n\r\n_FOR_EACH_EVENT_HANDLER is invoked in various places whenever hooks and\r\nevent handlers are defined or instanciated.\r\n\r\nA plugin base class EventHandlerBasePlugin is introduced that implements empty\r\nnoop methods for all event handlers.\r\n\r\nEvent handler documentation was moved to definition of _FOR_EACH_EVENT_HANDLER\r\nafter the hooks' explicit definitions have been removed.\r\n\r\nI am aware of the intention to provide documentation of the hooks in the Plugin class. But leaving the documentation and the description of the event handlers in `kaleidoscope::Plugin`'s definition would \r\nrequire to duplicate the signature of the hooks and thus generate a redundancy.\r\n\r\nIt might be worth considering if the `_FOR_EACH_EVENT_HANDLER` macro (or how ever you intent to call it) deserves its own header, directly in `src`). The user could be pointed to this header for information about the event handlers that are available in class `Plugin`."},"comment":{"url":"https://api.github.com/repos/keyboardio/Kaleidoscope/issues/comments/388165732","html_url":"https://github.com/keyboardio/Kaleidoscope/pull/318#issuecomment-388165732","issue_url":"https://api.github.com/repos/keyboardio/Kaleidoscope/issues/318","id":388165732,"user":{"login":"noseglasses","id":29701465,"avatar_url":"https://avatars3.githubusercontent.com/u/29701465?v=4","gravatar_id":"","url":"https://api.github.com/users/noseglasses","html_url":"https://github.com/noseglasses","followers_url":"https://api.github.com/users/noseglasses/followers","following_url":"https://api.github.com/users/noseglasses/following{/other_user}","gists_url":"https://api.github.com/users/noseglasses/gists{/gist_id}","starred_url":"https://api.github.com/users/noseglasses/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/noseglasses/subscriptions","organizations_url":"https://api.github.com/users/noseglasses/orgs","repos_url":"https://api.github.com/users/noseglasses/repos","events_url":"https://api.github.com/users/noseglasses/events{/privacy}","received_events_url":"https://api.github.com/users/noseglasses/received_events","type":"User","site_admin":false},"created_at":"2018-05-10T19:51:36Z","updated_at":"2018-05-10T19:51:36Z","author_association":"CONTRIBUTOR","body":"> The one thing I noticed is that you snuck in an architectural change, moving some code from Plugin to EventHandlerBasePlugin. If that's not fundamentally tied to the refactoring being functional, I'd prefer to consider that as a separate proposal.\r\n\r\nIt's tied because this is the only way to use the `_FOR_EACH_EVENT_HANDLER` macro to define `Plugin`'s event handler methods and at the same time allowing that one single method, namely `onSetup` is overridden to support the legacy plugin API. That's only possible by using inheritance."}}
{"actor":{"display_login":"noseglasses"}}