search

noiseorchestra / jacktrip_pypatcher

Python scripts to autopatch a JackTrip hubserver
GNU General Public License v3.0
6 stars 1 forks source link

Refactor one client patching #87

Open sandreae opened 3 years ago

sandreae commented 3 years ago

Still need a couple more tests...

sandreae commented 3 years ago

Need to test this once more now that I moved the disconnect and reconnect methods, the server's in use today though, so will do it tomorrow. Once we've merged this we can rewrite the pypatcher tests to be more thorough +1.

sandreae commented 3 years ago

I've tested this branch out more, up to 5 clients and booting from the automated script, all seems to be fine. Would be good to merge this if you're happy with it @madwort .

madwort commented 3 years ago

Sorry this is still hanging around - will try to make time to review it soon!

sandreae commented 3 years ago

Hey, thanks for the review, I started fixing the comments then wondered if the naming was a bit off actually. I feel something like this is a bit clearer and concise. What do you think?

These are the main publicly called module functions:

disconnect_all(...) connect_ports(...) set_darkice_connections(...) set_lounge_music_connections(...) <-- new set_client_connections(...) set_all_connections(...) make_connections(...) make_all_connections(...)

These are private functions (in which case they could be prefixed with _, right?):

connect_darkice_one_client(...) set_darkice_connections_one_client(...) connect_one_client(...) set_connections_1_clients(...) connect_two_clients(...) set_connections_2_clients(...) connect_three_clients(...) set_connections_3_clients(...)

madwort commented 3 years ago

Hey, thanks for the review, I started fixing the comments then wondered if the naming was a bit off actually. I feel something like this is a bit clearer and concise. What do you think?

yeah, good thoughts!

These are the main publicly called module functions:

disconnect_all(...) connect_ports(...) set_darkice_connections(...) set_lounge_music_connections(...) <-- new set_client_connections(...) ~set_all_connections(...)~ make_connections(...) ~make_all_connections(...)~

yeah, this looks good, maybe we could go further & find a better word for "set" too, just thinking, with the current 2-phase thing ("set" them, then "make" them) maybe it should be "generate" (i.e. "generate" them, then "make" them), not sure, there's probably a better word that I'm missing...

These are private functions (in which case they could be prefixed with _, right?):

Ah, probably, yes, I'm not the most python-ish person, I'm used to either having explicit private methods - sounds good to differentiate them though.

connect_darkice_one_client(...) ~set_darkice_connections_one_client(...)~ connect_one_client(...) ~set_connections_1_clients(...)~ connect_two_clients(...) ~set_connections_2_clients(...)~ connect_three_clients(...) ~set_connections_3_clients(...)~

I'm not sure - if we're sticking with the "set" verb above, and these are sub-functions of that, they should probably stay as "set", although e.g. set_client_connections_one_client might be better?

sandreae commented 3 years ago

yeah, this looks good, maybe we could go further & find a better word for "set" too, just thinking, with the current 2-phase thing ("set" them, then "make" them) maybe it should be "generate" (i.e. "generate" them, then "make" them), not sure, there's probably a better word that I'm missing...

Yeh, I don't like set in this context either, generate could be good.

I'm not sure - if we're sticking with the "set" verb above, and these are sub-functions of that, they should probably stay as "set", although e.g. set_client_connections_one_client might be better?

Yeh, I actually thought they should be differentiated a bit more because of the parent / child nature of their relationship. If we use a _ though with the private methods I think this is enough.

I'll make these changes and see how it looks.

sandreae commented 3 years ago

I kept with set in the end just cos generate is too long...

lounge_music is refactored, which makes for some nicely readable code here:

https://github.com/noiseorchestra/jacktrip_pypatcher/blob/66da5507bfee4254b0a92b7e77fe623de4735ca8/jacktrip_pypatcher.py#L108-L114

It is a fairly major change though so I do wanna spin up a server to test all this is working as expected.

sandreae commented 3 years ago

What do you think about the code in all of these connection methods?:

https://github.com/noiseorchestra/jacktrip_pypatcher/blob/66da5507bfee4254b0a92b7e77fe623de4735ca8/jack_client_patching.py#L63-L68

Should they be re-written like so (maybe with abbreviated naming?):

def _set_darkice_connections_one_client(self, jacktrip_ports, darkice):  
     self.connections.append((self._jacktrip_receive(jacktrip_ports[0]), self._darkice_send(darkice)))

I think I prefer the former for readability, but the latter is many fewer lines of code, and with abbreviated names could be fairly compact....

sandreae commented 3 years ago

I mean, if we do that it's hardly worth having the helper method, as it's only used once here....

https://github.com/noiseorchestra/jacktrip_pypatcher/blob/66da5507bfee4254b0a92b7e77fe623de4735ca8/jack_client_patching.py#L172-L181