[thanks4opensource]

This pull request adds new SequencerClientBase.get_address() and SequencerClientBase.fd() methods, and tests/test_sequencer_address.py for the former. Also a modified examples/connect.py demonstrating the new .get_address() capabilities.

SequencerClientBase.fd() is a public wrapper for ._fd. Access to the ALSA file descriptor is needed for programs doing select() (e.g. with stdin), and ones that use GUI toolkits with gui.add_fd(); gui.run() semantics. I have successfully used SequencerClientBase._fd is both cases, in addition to snd_seq_poll_descriptors() in C/C++ programs. Do you see any problem with doing this? The new .fd() method makes it "official", as well as future-proofing in case of ALSA library changes.

SequencerClientBase.get_address() returns an Address given various forms of client and port inputs such as "client-name:port-name" strings, separate "client" and "port" strings, int client and port IDs, tuples of these, Address objects, and mixes/combinations of them. It returns an Address only if the specified client/port exists, raising an exception if not. It accepts prefixes/abbreviations/substrings of client and port names, and matches current Address() -- and the underlying ALSA C library's snd_seq_parse_address() -- semantics of returning Address(client_id, 0) if given a client without a port, but only if the client exists and has a port 0 (almost universal, but not enforced by the library).

An irony is that I have submitted alsa-project/alsa-lib#528 which includes much (but not all) of this functionality directly in the ALSA C library. See that pull request for the reasoning that motivated these proposed enhancements. I originally experimented with including the "name-or-numeric-ID" capability of the enhanced snd_seq_parse_address() directly into Address by adding an optional SequencerClientBase parameter to an Address.__new__() overload. But I am unsure when (if ever) the alsa-lib pull request will be merged, I didn't want python-alsa-midi to depend on an updated release of the ALSA library, and it seemed cleaner and more Pythonic to implement it as a SequencerClientBase method anyway. Given SequencerClientBase.list_ports() this was fairly easy to do, although overload input validation and checking for port/client existence required significant additional code. I also considered merely returning None for non-existing client/ports but again felt exceptions better matched the rest of python-alsa-midi's semantics.

Included is a unittest program, tests/test_sequencer_address.py which extensively checks many (currently 4179) permutations of input types and expected successful returns or exception failures. Both the new and all existing tests run without error in this branch. A modified examples/connect.py demonstrates connecting clients/port by name instead of by numeric IDs.

Please consider merging this pull request. Feel free to modify/re-architect it as you see fit, hopefully retaining all of the current capabilities. Thank you.

[Jajcus]

Thank you for your merge requests. A few comments...

• These should be probably two Merge Requests, or at least two separate commits.
• Tests fail, as your tests do not integrate with existing test infrastructure (tox & pytest) at all. That is why the CI checks fail for the MR, and it should be verified with tox before pushing.

And now about the actual changes:

For the fd() method... I don't like it being a method. Maybe, if it was .fileno(), as in IOBase (with the same semantic), it could be argued for, but fd()? Not even get_fd()? But get_* functions here are usually direct interfaces to the underlying ALSA API, so it would be also confusing.

To me the most reasonable would be to make this a read-only property. client.fd, typed 'int | None'.

For the get_address(). I think the name is confusing, sounds like it is getting address of the client. Maybe resolve_address()? Sounds more like what the function is doing.

[thanks4opensource]

Thanks for reviewing the pull request. I've made changes as per your comments. Continuing the discussion ...

I have no strong preferences regarding the fd() or "address()" methods. I changed get_address() to resolve_address(). I changed fd() as follows:

+    @property
     def fd(self):
         "Returns the ALSA sequencer file descriptor number."
         return self._fd

Is this acceptable? It mandates use as client.fd (without "()") and raises an exception on client.fd = value. The only thing I don't like about it is that help(client.fd) doesn't display the docstring, nor does anything else I could find. I couldn't figure out the required syntax for typed 'int|None', etc, so if there's a better way using that please tell me the exact code (or feel free to change/add it yourself).

I apologize for the tests issue. I had little prior exposure to pytest, and had never heard of tor. I tried variants of python3 setup.py test without success and concluded there was no built-in way to run the tests in the directory. I resorted to doing:

$ cd tests
$ for test in *.py ; do
> echo -n $test '  '
> python3 $test && echo $?
> done

which worked for my new test_sequencer_address.py but of course did nothing with the others as I now see. I would have known that if I'd looked at their code.

I'm a huge believer in unittests but have no love for test frameworks like pytest/tor/etc. IMO they add many layers of complexity and rigorous demands while providing little benefit in return. Let me write whatever form of test I want as long as it runs silently returning 0 if successful to whatever process has forked/execl'd it, and outputs error text and returns non-0 if not. Your opinion may differ, and since this is your repo I modified the code to run at least passably well under tor/pytest.

With one exception: I strongly object to tor's style checker. As a tiny, poor example:

    sender = client.get_address(args.sender)
    dest   = client.get_address(args.dest  )

Is inarguably more readable than:

    sender = client.resolve_address(args.sender)
    dest = client.resolve_address(args.dest)

The human visual system (really that of all animals) has evolved to rapidly recognize patterns. You can glance at the first form and immediately see that some method is being called twice, with different members from the same object, and the result is being assigned to two different variables. You can look more closely if you're interested in the details, but in the second form you have to do that to understand what's going on.

I'll stop lecturing here, but the bottom line is it's been my policy to try to match the style of code I'm modifying and use my own for entirely new code. It's a gray area when I'm adding a large block of new code to an existing source file, and I erred on the side of "mine" in what I added to client.py, which I've corrected in this new commit. My apologies.

But for now I've left test_sequencer_address.py largely alone which of course raises hundreds of style errors from tor. So I have to ask: Is there any way to turn off the style checking, possibly just for this specific source file? Would you be willing to do so? Or is the decision out of your hands -- does whoever runs PyPi/pip insist on this level of compliance (even for a unittest that doesn't get installed by a normal user) before they'll accept the package? BTW, I'm getting dire warnings that setup.py is deprecated (along with some for tor and pytest) which is among the many reasons I have little respect for these fad-of-the-year software development environments/requirements.

And more generally, I have to ask what your level of interest is in receiving this kind of unsolicited new feature merge request? This is an old project for you and I'm sure you've moved on to other things and have little time to devote to it. You've been very responsive here, especially compared to https://github.com/alsa-project/alsa-lib where the backlog of pull requests (yes, I'm aware of the issue with mine) convinces me that it's effectively a dead/abandoned project. (And aren't people saying that ALSA itself has been deprecated, first by JACK and now by pipewire?)

For myself, I had a window of time to work on this, and it's rapidly closing. I had for years wanted to consolidate my usage of ALSA, ppaez/alsaseq, rtmidi, etc. in C/C++/Python -- and fix the problems I was having with them -- in the dozen or so personal applications and utilities I've written and maintain. I also wanted to solve the years of frustration I've had with not being able to address ALSA clients and ports by their names.

So again, the bottom line: If you'll give me a clear indication that you'll merge this pull request -- and the next, final one (a full-featured, improved Python replacement for aconnect/aconnect.c including another, purely orthogonal addition to SequencerClientBase which it requires) -- I'll spoil the maintainability of test_sequencer_address.py by "fixing" its whitespace/indentation, although again I'd much rather find a way to get tor off my back and leave it as is. If not, I can just let the code sit in my own forked repo. I've put in too much time and work on it to keep it privately to myself when I think others could benefit from it. That's always the decision I have to make when I start something like this. I could have finished it all in 10% of the time if it was only for my own personal use: Sloppy code, no unittests (which always take up 3x the time as the actual code they test), find and fix any bugs as they come up in my day-to-day use. But it has to be more rigorous if it's being distributed to the outside world, especially as part of a project like yours which, as I've said before, I consider to have a very high standard of software quality.

Please tell me how I should proceed, and again thanks for this repo and the time you've spent on it.

[thanks4opensource]

Update:

• Whitespace/indentation changes, directives, and exclusions
  for for tox/pytest/isort.
• All tests pass without error.

[Jajcus]

a typo is introduced here

[Jajcus]

please not

[Jajcus]

docstring should go before the imports - it should be the first statement in the module

[Jajcus]

I will implement this differently (as a .fileno() method), you can drop it.

[Jajcus]

Still does not match the way other tests are written.

Single test function instead of actual unit tests and pytest framework is not fully used as it could be here.

But this does some thorough testing and works under pytest, so I can accept it.

[Jajcus]

Should be 'arg1' here, not 'arg2' (arg2 is None)

[Jajcus]

please no

[Jajcus]

For the fd – I think we should implement fileno() method instead, as that is what select() and poll() support natively. I will do that.

For the tests and coding standard. The tool is 'tox' not 'tor' and your style checking preferences are irrelevant here: there there is a well defined coding style for Python (PEP 8) and it should be followed. Consistency is more important than personal preferences, which will always differ. Inconsistent formatting can cause serious objective problems when merging or reviewing code. And yes, the tests are no exceptions here. There are tools handle the formatting for you – either built-in in the code editor (probably via a language server) or stand-alone command-line ones.

Your tests still fail when ALSA is not available in kernel (which may be the case in many CI platforms, like here on GitHub). Test functions that require actual kernel functionality and not just the ALSA library should be guarded with a @pytest.mark.require_alsa_seq annotation.

In the resolve_address() implementation there is an actual bug – see the review above. The test should be also updated to catch that.

And more generally, I have to ask what your level of interest is in receiving this kind of unsolicited new feature merge request? This is an old project for you and I'm sure you've moved on to other things and have little time to devote to it.

I am very happy someone is interested in my project (which seemed ignored most of the time) and wants to contribute. I still plan to maintain this and even add more functionality (like MIDI 2.0), but I was not able to give this much of my time recently.

I will implement the fileno() function myself (similar to you original fd(), but compatible with IOBase semantics) and I can merge your resolve_address() implementation when you fix the remaining issues. Please squash the commits and remove the fd() stuff, so history can be clean.

The improved aconnect implementation seems interesting too, but I would need to see the details to say if it can be included here.

[thanks4opensource]

commit message:
client: add resolve_address() and fileno()

Parses various input overloads such as 'client-name:port_name',
('client-name','port-name'), etc., returns Address. Raises exception
if Address(client_id, port_id) doesn't exist. Added new fileno()
public accessor. Added tests/test_sequencer_address.py. Modified
examples/connect.py to accept 'client-name:port-name'
Update:

• Changed get_address() to resolve_address()
• Added @Property to fd() to make read-only fd
• Fixed test_sequencer_address.py to run under tox/pytest
• Changed whitespace,etc to pass tox style/flake8, except
  in test_sequencer_address.py
  Update:
• Whitespace/indentation changes, directives, and exclusions
  for for tox/pytest/isort.
• All tests pass without error.
  Update:
  client.py:
• Removed "@Property\ndef fd(self):"
• Replaced with Jajcus-provided "fileno(self)"
• Removed typo/bug, redesigned/rewrote argument validation in resolve_address()
• Fixed typo introduced in docstring
  test_sequencer_address.py
• Fixed bug, use of non-existent variable
• Removed unused variable
• Removed "# flake8: noqa"
• Reformatted code with autopep8
• Moved file-level docstring to top of file
• Added "import pytest" and "@pytest.mark.require_alsa_seq"
• Added more "# isort: skip" (imports in correct order, still produce errors)
• Added "# noqua: F824" (global variables are used, flake8 flags incorrectly)
  tox.ini
• Removed "-exclude = tests/test_sequencer_address.py

Comments:

a typo is introduced here

My apologies. Fixed.

Should be 'arg1' here, not 'arg2' (arg2 is None)
In the resolve_address() implementation there is an actual bug – see the
review above. The test should be also updated to catch that.

Thanks for catching this typo. There were several others, and the entire logic of argument validation was messy, although it worked because this error was caught later, and test_sequencer_address.py does check for it. I rewrote/improved the entire section, and the new code still passes all the tests.

docstring should go before the imports - it should be the first statement in the module

I question whether an exception would be made for modules that have a if __name__ == "__main__": section at their end and #!/usr/bin/env python3 as their first line so the file can be made executable and run standalone by users in addition to being imported into other programs/modules, but that's irrelevant here.

I will implement this differently (as a .fileno() method), you can drop it.

I "merged" your code into this commit.

# flake8: noqa
Please not

exclude = tests/test_sequencer_address.py
please no

But this does some thorough testing and works under pytest, so I can accept it.

Thank you. I ran the file through autopep8 which "fixed" the style violations.

Because it's still not 100% pytest-approved there were still a few specific # isort: skip and # noqua: F824 escapes required (imports are in correct order, system/3rd-party/rep -- which is something I always do myself -- but isort still complained and I got tired of trying a dozen permutations to make it happy) (the globals are used, flake8 just doesn't understand the code because it's not in individual tests) (can't be, requires clients/ports to be created for before) (yes, I've seen some of the "setup/teardown" stuff, again much more trouble than it's worth).

BTW, did you notice the "time.sleep(2)" I had to add? Sometimes the individual tests would run before the clients/ports were actually ready in ALSA -- mostly when other errors occurred and/or because the tests are run in parallel -- and then everything would error out instantly.

The tool is 'tox' not 'tor

I can't keep these systems I never use straight.

Test functions that require actual kernel functionality and not just the ALSA library should be guarded with a @pytest.mark.require_alsa_seq annotation.

This was completely opaque to me. I've added the guard and of course it passes my local tox runs, but I'll see what happens when I push to GitHub. (Still getting errors, seemingly unrelated about "Union" type-checking being undefined. Sigh. If the CI would tell me something useful like "your code runs on x86-64 but not on ARM because ..." it would be one thing, but having local builds work and then trying to hunt down things like this at a 20 minute write-commit-push cycle is more than I'm willing to undertake.)

Please squash the commits and remove the fd() stuff, so history can be clean.

I rate myself about 3 out of 10 in git knowledge. I can do basic things, but everything I've seen about rebasing/squashing/etc (any question on StackOverflow generates 50 different ways to do the same thing, with 49 of them giving dire warnings why you shouldn't use the others) convinces me that a single typo will permanently corrupt my repo beyond repair. (My favorite git claim is, "It's very hard to lose your work with git." Yeah? Edit a file, mistake git restore for the similarly-named git revert or git reset and, boom!, your code is gone.)

You obviously know far more git than I do. I researched and there seem to be ways to grab a single commit from another repo, either directly from GitHub or other server or from a local clone. You seem to delete branches once you've merged them into main, so please do something like that with my branch(es) and let me keep my own history, safely. (I actually keep a lot more history, outside of git, but that's another subject.)

Unless there are some more actual bugs -- which I'll attempt to address -- I intend this to be my last push of this branch. I've found another piece of functionality (actual useful code for the end user) that I want to add to aconnect.py, and when I finish that, hopefully in a few days, I'll submit another pull request which you can review, rework, or reject as you desire. All the secondary issues (tox/pytest/CI/etc) should work at least as well (or poorly) in that commit as they currently do here.

[Jajcus]

BTW, did you notice the "time.sleep(2)" I had to add?

I had to use similar constructs in my own tests too. It sucks, but we cannot control kernel timings. If the value is long enough for the tests to reliably pass and short enough not to increase test duration too much it is ok.

I have rebased the code, added two minor commits and pushed as a new branch, so this MR is now superseded by #30

Thank you for your contribution!