larsimmisch / pyalsaaudio

ALSA wrappers for Python
http://larsimmisch.github.io/pyalsaaudio/
Other
216 stars 74 forks source link

assorted improvements #123

Closed ossilator closed 1 year ago

ossilator commented 1 year ago

the commits are atomic and should be self-explanatory.

RonaldAJ commented 1 year ago

Dear Oswald,

You moved documentation from doc_strings to the rst-files. I can see that having the documentation in one place has it advantages. But I was wondering what the impact of that move is on the help function? I suppose I can find the answer myself, but I guess you know.

Kind regards, Ronald

RonaldAJ commented 1 year ago

Small points on the documentation:

xruns: first time I encounter this terminology, it is probably better to mention both underrun and overrun nearly everywhere to improve readability. I did notice you do explain the xrun terminology, but the overrun and underrun terminolgy is kind of self-explanatory where xrun is not.

c'tor: short for constructor?

I can't judge the technical changes, so I won't.

ossilator commented 1 year ago

But I was wondering what the impact of that move is on the help function?

well, the pydoc-based help() will be obviously gone. and that's kind of the point - better have no docu than an incomplete/misleading one (which might prevent the user from looking for the official docs). do you see a problem with the reasoning?

ossilator commented 1 year ago

xruns: first time I encounter this terminology, [...]

that's unexpected to me, as the alsa api and everything that surrounds it (the code and docu) is soaked with that term. it isn't used outside the document that defines it and some commit messages, so i think that's ok?

c'tor: short for constructor?

yeah. i'm coming from c++, where this is pervasive. i guess this is ok in a developer-targeting hidden comment?

RonaldAJ commented 1 year ago

xruns: first time I encounter this terminology, [...]

that's unexpected to me, as the alsa api and everything that surrounds it (the code and docu) is soaked with that term. it isn't used outside the document that defines it and some commit messages, so i think that's ok?

I would disagree. In my view the audience for pyalsaaudio are also Python developers not necessarily well versed in ALSA. For simple applications I would think the intent is to hide the full complexity of ALSA. Many people would try to get the example code working, and if they fail they won't try ALSA directly but simply give up.

c'tor: short for constructor?

yeah. i'm coming from c++, where this is pervasive. i guess this is ok in a developer-targeting hidden comment?

While in Python everything is a class. Often people just talk about the init method rather than the constructor. In my perception the abbreviation c'tor is not used or at least not often. Also there are many Python programmers without a computer science background.

RonaldAJ commented 1 year ago

But I was wondering what the impact of that move is on the help function?

well, the pydoc-based help() will be obviously gone. and that's kind of the point - better have no docu than an incomplete/misleading one (which might prevent the user from looking for the official docs). do you see a problem with the reasoning?

People expect the help function to produce the doc-string in Python. Normally the documentation is largely generated from the doc-strings in the code. Keeping the documentation close to the code makes it more likely that it is updated when there is a code/interface change.

Apart from that it is clear that this is not the original choice made for this library, which indicates to me that the availability of the help functionality was intended.

ossilator commented 1 year ago

In my view the audience for pyalsaaudio are also Python developers not necessarily well versed in ALSA.

the audience of commit messages and hidden docu are developers of pyalsaaudio, not its users, so this point is irrelevant.

and as far as the terminology document goes, i think it shouldn't surprise anyone that it defines terms the reader was not familiar with yet ...

ossilator commented 1 year ago

People expect the help function to produce the doc-string in Python.

what people? i certainly don't - i try to find the official documentation online.

Apart from that it is clear that this is not the original choice made for this library, which indicates to me that the availability of the help functionality was intended.

a quick check of the git history reveals that lars originally used pydoc. however, he later switched to sphinx. at this point, the docstrings are simply historical cruft, as evidenced by the fact that he didn't keep them up to date.

my first thought when i saw the duplication was to revive the pydoc and drop the rst file, but i quickly found that sphinx permits for so much better markup, which is presumably why lars chose it.

i suppose it would be possible to get the best of both worlds by using https://pypi.org/project/sphinx-c-autodoc/ or https://github.com/jnikula/hawkmoth, but for such a small project this seems like complete overkill to me.

RonaldAJ commented 1 year ago

In my view the audience for pyalsaaudio are also Python developers not necessarily well versed in ALSA.

the audience of commit messages and hidden docu are developers of pyalsaaudio, not its users, so this point is irrelevant.

I wasn't referring to commit messages or hidden documentation. I referred to the documentation and the help functionality. I think being more explicit and writing "buffer overruns and underruns" in the help and documentation saves many developers a look-up action and keeps them in flow. Some terminology is unavoidable other can be avoided.

and as far as the terminology document goes, i think it shouldn't surprise anyone that it defines terms the reader was not familiar with yet ...

Agreed. But it is better not to introduce terminology that strictly speaking isn't needed. And I do understand that xrun is a handy shorthand for ALSA developers. So maybe it fits in the terminology list, but personally I would steer away from using it elsewhere in the documentation and help functionality.

RonaldAJ commented 1 year ago

People expect the help function to produce the doc-string in Python.

what people? i certainly don't - i try to find the official documentation online.

As I indicated given the size of the project it is a defendable choice here. But assuming you are the average user is a wrong assumption. I spend a few years in GUI development, and people not only use the documented behavior they also use the undocumented behavior. Also students, I had a few, need to be taught to look up the official documentation, but many of them prefer youtube videos showing how things are done. Which personally I find a very ineffective way as you can't drill down to the essence very quickly. But it is how some people work. I guess some people also rely on their IDE to show them the doc_string. So the choice you make is not without consequences.

Apart from that it is clear that this is not the original choice made for this library, which indicates to me that the availability of the help functionality was intended.

a quick check of the git history reveals that lars originally used pydoc. however, he later switched to sphinx. at this point, the docstrings are simply historical cruft, as evidenced by the fact that he didn't keep them up to date.

my first thought when i saw the duplication was to revive the pydoc and drop the rst file, but i quickly found that sphinx permits for so much better markup, which is presumably why lars chose it.

i suppose it would be possible to get the best of both worlds by using https://pypi.org/project/sphinx-c-autodoc/ or https://github.com/jnikula/hawkmoth, but for such a small project this seems like complete overkill to me.

That could well be the case. I didn't write it. So I guess @larsimmisch is the better judge of that.

That being said, I think ALSA expertise is welcome here, and you seem to bring that!

ossilator commented 1 year ago

I referred to the documentation and the help functionality.

and i tried really hard to point out that these terms don't appear in any user-facing docu, except 'xrun' within a small self-contained document, which is going to be read in full anyway.

But it is better not to introduce terminology that strictly speaking isn't needed.

this argument can be easily driven ad absurdum.

also, i don't think it's reasonable to "spare" readers from terms that are actually common in the domain they are entering; they might well run into them again.

I guess some people also rely on their IDE to show them the doc_string.

i suppose this may be true, but see my very first response above.

RonaldAJ commented 1 year ago

I referred to the documentation and the help functionality.

and i tried really hard to point out that these terms don't appear in any user-facing docu, except 'xrun' within a small self-contained document, which is going to be read in full anyway.

Apologies, I see, you are right, I was to quick.

also, i don't think it's reasonable to "spare" readers from terms that are actually common in the domain they are entering; they might well run into them again.

I think most users don't want to learn about the internals. They just want to get a recording or playback a sound. Which requires a correct ALSA configuration and a working script. Which is quite a challenge.

But I sense we are not going to agree on this one. And it is not relevant, because now it is clear that xrun is not in the user facing documentation.

ossilator commented 1 year ago

I think most users don't want to learn about the internals.

most probably. :laughing: but the reality is that it seeps through; you'll find lots of questions on stackoverflow where people ask about error messages and stuff.

also, while the term is used a lot in alsa, it's not "alsa-speak" per se. i think the average audio user can really benefit from knowing it.

ossilator commented 1 year ago

testing the draining fix would just require adding out.close() to the end of playbacktest.py. i amended the commit to include that. and using out.drain() instead would test that in turn, though including that in the manual test doesn't really work due to it being functionally mutually exclusive with the former (and i don't think an option for that would really fit the current character of the test/example). the catch is that without a loopback and concurrent recording it's basically impossible to reliably prove anything that way. my application actually does that, which is how i discovered the problem. furthermore, the linux kernel is also kinda buggy and will playback garbage while draining. i have a patch lined up for that as well.

the overrun code is also tested in practice by my setup occasionally triggering it (python really is a tad slow and unpredictable - maybe not the optimal platform for real-time high-frequency data sampling ...). i suppose one could test this somewhat easily by just inserting some delays into i/o sequences, but given the lack of an actual autotest suite, i'm somewhat unenthused about the prospect of having to do it myself. :stuck_out_tongue_winking_eye: hacking the manual tests for quick one-off verification seems easy enough, but i wouldn't include that in the package for the same reason as above. but note that just starting the stream already provides coverage for most of the recovery code.

was there anything particular about your test system? in principle, this sounds like something easy to recreate, even if only in a vm.

larsimmisch commented 1 year ago

Thanks; way too late.

ossilator commented 11 months ago

oh my, why did you squash-merge this?! it was perfectly atomized ...