spatialaudio / jackclient-python

🂻 JACK Audio Connection Kit (JACK) Client for Python :snake:
https://jackclient-python.readthedocs.io/
MIT License
132 stars 26 forks source link

Implement Port.alias() property. #45

Closed jofemodo closed 6 years ago

jofemodo commented 6 years ago

Uses "int jack_port_get_aliases(const jack_port_t port, char const aliases[2])" from the jack C library.

mgeier commented 6 years ago

Thanks for this PR!

What about adding set_alias() and unset_alias()? This way we could easily test if/how the alias property works.

There seem to be up to two aliases per port, what about returning a list of str instead of a single str?

jofemodo commented 6 years ago

I've implemented this feature because i need it. I've no need for the set_alias and unset_alias functions, although it's pretty simple to implement. I will do.

Also, i will return a list of aliases ;-)

jofemodo commented 6 years ago

Done! ;-)

mgeier commented 6 years ago

Thanks a lot for the changes! Currently, I don't have time to review, I'll come back to you after the weekend.

jofemodo commented 6 years ago

I just added a little test to examples for easier reviewing ;-)

mgeier commented 6 years ago

The API of jack_port_get_aliases() seems quite strange to me, because if it returns 1, it doesn't tell us which of the two aliases is set ... or am I missing something?

Returning a list which may have empty strings in it (or even None values) seems a bit strange and un-Pythonic to me. What about simply returning a "list of aliases" with 0, 1 or 2 elements, depending on the number of actual aliases?

mgeier commented 6 years ago

Another thought: We could even return a set instead of a list?

jofemodo commented 6 years ago

OK to the first part. Returning a set is not a good idea because the aliases sorting is lost.

mgeier commented 6 years ago

Thanks for the changes!

Does a "sorted list of aliases" even make sense? Would you ever use the sorted-ness of the aliases in your code? And if we return 1-element lists, we already give up on the sorted-ness.

How would you use the aliases property in your code?

Anyway, I'm not totally convinced about using a set. If you are convinced about using a list, I'm fine with it.

jofemodo commented 6 years ago

OK! I've improved the code, reducing the redundance. Regarding the list/set question, i prefer returning a list. In my code is not important the order, as only one alias is set, but i can imagine a case where you would like to get the "first" or "last" alias ;-) Anyway, if you think it's better, i will change to a "set" ...

mgeier commented 6 years ago

OK, thanks for the changes, let's keep the list.

Now there are just a few formal issues left, regarding PEP-8 and PEP-257.

$ python3 -m pycodestyle src/jack.py 
...
src/jack.py:1451:14: E225 missing whitespace around operator
src/jack.py:1452:16: E225 missing whitespace around operator
src/jack.py:1453:19: E225 missing whitespace around operator
src/jack.py:1454:15: E225 missing whitespace around operator
src/jack.py:1457:22: E225 missing whitespace around operator
src/jack.py:1465:67: W291 trailing whitespace
...
$ python3 -m pydocstyle src/jack.py
...
src/jack.py:1463 in public method `set_alias`:
        D205: 1 blank line required between summary line and description (found 0)
src/jack.py:1463 in public method `set_alias`:
        D209: Multi-line docstring closing quotes should be on a separate line
src/jack.py:1470 in public method `unset_alias`:
        D205: 1 blank line required between summary line and description (found 0)
src/jack.py:1470 in public method `unset_alias`:
        D209: Multi-line docstring closing quotes should be on a separate line
src/jack.py:1470 in public method `unset_alias`:
        D400: First line should end with a period (not ',')
...

Please rename the example file, because files starting and ending with "test" should be reserved for automated tests (e.g. py.test looks for those files).

jofemodo commented 6 years ago

OK @mgeier! I hope everything is OK now ;-)

mgeier commented 6 years ago

Thanks for the changes!

Did you consider these comments of mine?

You are using tabs in the example file, which is a no-go. You should check for PEP-8 issues (e.g. with python3 -m pycodestyle examples/aliases.py). Ideally, you should use a plugin for your text editor to show those issues.

Finally, please also use .format() in the example.

jofemodo commented 6 years ago

OK! Let's see now ... cross my fingers ;-D

mgeier commented 6 years ago

Thanks a lot @jofemodo!

jofemodo commented 6 years ago

It's been a pleasure, @mgeier ;-)