{"action":"created","issue":{"url":"https://api.github.com/repos/spatialaudio/jackclient-python/issues/16","labels_url":"https://api.github.com/repos/spatialaudio/jackclient-python/issues/16/labels{/name}","comments_url":"https://api.github.com/repos/spatialaudio/jackclient-python/issues/16/comments","events_url":"https://api.github.com/repos/spatialaudio/jackclient-python/issues/16/events","html_url":"https://github.com/spatialaudio/jackclient-python/pull/16","id":80481117,"number":16,"title":"Inheritable Method Callbacks","user":{"login":"amstan","id":160217,"avatar_url":"https://avatars.githubusercontent.com/u/160217?v=3","gravatar_id":"","url":"https://api.github.com/users/amstan","html_url":"https://github.com/amstan","followers_url":"https://api.github.com/users/amstan/followers","following_url":"https://api.github.com/users/amstan/following{/other_user}","gists_url":"https://api.github.com/users/amstan/gists{/gist_id}","starred_url":"https://api.github.com/users/amstan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/amstan/subscriptions","organizations_url":"https://api.github.com/users/amstan/orgs","repos_url":"https://api.github.com/users/amstan/repos","events_url":"https://api.github.com/users/amstan/events{/privacy}","received_events_url":"https://api.github.com/users/amstan/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-05-25T10:06:40Z","updated_at":"2015-05-26T17:50:55Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/spatialaudio/jackclient-python/pulls/16","html_url":"https://github.com/spatialaudio/jackclient-python/pull/16","diff_url":"https://github.com/spatialaudio/jackclient-python/pull/16.diff","patch_url":"https://github.com/spatialaudio/jackclient-python/pull/16.patch"},"body":"This is an example what i meant by my comment at https://github.com/spatialaudio/jackclient-python/pull/4#issuecomment-77734369.\r\n\r\n> More importantly, I think there is even a quite bad disadvantage, since all callbacks are called, even if they are empty functions. At least for the process callback, this adds unnecessary overhead.\r\n\r\nThe only function which could cause a performance penalty is process, and that one needs to be overridden anyway. While I could implement something that detects that nothing inherited most callbacks so don't set_*_callback on them, all the non-process callbacks happen very rarely so I don't think it's worth it.\r\n\r\n> their code gets more explicit and more readable\r\n\r\nSpamming set\\_*\\_callback in my \\_\\_init\\_\\_ is not my idea of readable when i have other stuff to worry about there too.\r\n\r\nFor the final merge I'm proposing everything under PythonicClient to the Client proper. It even has a way to disable the extra functionality by passing method_callbacks=False. I could work on the docstrings too a little so all set\\_\\*\\_callback methods refer to the on_* methods for the callback signatures (and move the signatures there while I'm at it)."},"comment":{"url":"https://api.github.com/repos/spatialaudio/jackclient-python/issues/comments/105617128","html_url":"https://github.com/spatialaudio/jackclient-python/pull/16#issuecomment-105617128","issue_url":"https://api.github.com/repos/spatialaudio/jackclient-python/issues/16","id":105617128,"user":{"login":"mgeier","id":705404,"avatar_url":"https://avatars.githubusercontent.com/u/705404?v=3","gravatar_id":"","url":"https://api.github.com/users/mgeier","html_url":"https://github.com/mgeier","followers_url":"https://api.github.com/users/mgeier/followers","following_url":"https://api.github.com/users/mgeier/following{/other_user}","gists_url":"https://api.github.com/users/mgeier/gists{/gist_id}","starred_url":"https://api.github.com/users/mgeier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mgeier/subscriptions","organizations_url":"https://api.github.com/users/mgeier/orgs","repos_url":"https://api.github.com/users/mgeier/repos","events_url":"https://api.github.com/users/mgeier/events{/privacy}","received_events_url":"https://api.github.com/users/mgeier/received_events","type":"User","site_admin":false},"created_at":"2015-05-26T17:50:54Z","updated_at":"2015-05-26T17:50:54Z","body":"Thanks a lot for this PR!\r\nI like some of the commits, but not all of them ...\r\n\r\naa5472be890ff0a1b3ab4cd4fd3510d8f8cc2b4e:\r\n\r\nThis is a really nice idea, I didn't like the return values very much myself.\r\n\r\nBut if we do this, we should go all the way and completely drop the return values from the callbacks.\r\nIf an exception occurs, CFFI will automatically return the value from the `error` argument, so we could unconditionally return the success value.\r\nThere would be no need for the module constants `SUCCESS` et al. anymore, they could become `_SUCCESS` or similar.\r\n\r\nThere is only one case where I'm unsure, namely the process callback.\r\nI'm not sure if \"don't call again\" is really meant as an error or just as an equally reasonable alternative to \"call again\".\r\n\r\nIf a user wants to stop the callback without there being an exception already, she will have to raise an exception herself.\r\nWhat exception shall we suggest our users to raise in this case?\r\n\r\nA few exception classes that come to mind: `EOFError`, `RuntimeError`, `StopIteration` or simply `Exception`.\r\nIt doesn't really matter what they raise, but I think it would be good to provide a meaningful suggestion.\r\n\r\nIf you somewhat agree, it would be nice if you could make a separate PR for this, then we can discuss the details there.\r\n\r\n91116296b5ecec8a64536374971a7d36e3338db2:\r\n\r\nI like this idea!\r\nDo we even need the separate property if we pass it to the callback?\r\n\r\n479d37404c0da45b934860bda8ec5614d3ed7770:\r\n\r\nThis one I don't like.\r\n\r\nI don't agree that this is Pythonic, on the contrary, I think it's much too Java-esque!\r\n\r\nForcing those poor callback functions onto a class for no good reason doesn't seem like a good idea to me.\r\nEspecially the example is a bad example (or rather good for what I'm trying to say), since not a single callback *method* uses `self`, so there is no reason for them to be methods in the first place.\r\n\r\nI'm not saying that I want to forbid users to inherit from `jack.Client`, but if they do, they should register their callbacks explicitly.\r\nI think it's also quite unlikely that they actually need *all* the callbacks.\r\n\r\n> The only function which could cause a performance penalty is process, and that one needs to be overridden anyway.\r\n\r\nThat's not true (the first part is, but the second part isn't).\r\nE.g. one could implement some kind of JACK connection manager, without needing the process callback.\r\n\r\nWhile it's true that the only place where it actually matters performance-wise is the process callback, I still think that it's stylistically ugly to call all callback setters in the beginning (even if none is needed) and then call all those empty callbacks (even if none is needed).\r\n\r\n> Spamming set_*_callback in my __init__ is not my idea of readable when i have other stuff to worry about there too.\r\n\r\nThat's a fair point, but I guess most users will only need a handful of callbacks [citation needed ...].\r\nAnd those that need more can make a separate class like you showed in your commit, no need to mix the callback setup with other initializations ...\r\nI just don't think it's the right thing to do within the library.\r\n\r\nIf we can find an example application where deriving from `jack.Client` actually makes a difference for the better, we can add it there.\r\nBut we shouldn't call it \"Pythonic\".\r\n\r\nJust as a little side note: I don't like the names \"`on_*()`\" ... is there any \"true\" Python library (i.e. not something ported from another language) that uses this nomenclature?\r\n\r\n> It even has a way to disable the extra functionality by passing `method_callbacks=False`.\r\n\r\nI don't like this option.\r\nIf anything, it should be a separate class with a separate name, because the behavior is significantly different.\r\n\r\n48b150e802c6ffee92dc136f7f4f4784539d656b:\r\n\r\nI don't think that's an improvement at all, but of course that's highly subjective.\r\nThe scope of function arguments should be quite clear to the reader and I think using the same name in all functions actually makes them easier to read.\r\n\r\nIs there any role model for your suggestion?\r\n\r\nI guess we won't find anything like this in the standard library because the BDFL hates callbacks in general ..."}}