chaudum / rgain3

A Python 3 compatible fork of rgain -- ReplayGain tools and Python library
GNU General Public License v2.0
51 stars 10 forks source link

#26 broke import API: was this intentional? #29

Open smcv opened 4 years ago

smcv commented 4 years ago

In #26 various importable objects were moved around. Was this intentionally an API break? Other packages that import rgain3 as a library, such as https://github.com/rmcauley/rainwave, will presumably need the same changes that rgain3's own tests did.

Given rgain3's rather short history under that name, it's probably OK to be breaking API (and if you want to break API, now is the time!), but it seemed worth checking.

I've uploaded rgain3 1.0.0 to Debian's 'unstable' rolling release, but I'm going to stop it migrating to 'testing' (the alpha version of Debian 11) for the moment.

chaudum commented 4 years ago

In #26 various importable objects were moved around. Was this intentionally an API break? Other packages that import rgain3 as a library, such as https://github.com/rmcauley/rainwave, will presumably need the same changes that rgain3's own tests did.

Given rgain3's rather short history under that name, it's probably OK to be breaking API (and if you want to break API, now is the time!), but it seemed worth checking.

Looking back, I think releasing 1.0.0 was a mistake, since it obstructed the possible introduction of breaking changes with the new package name.

Thanks for bringing that up. I am now thinking of reverting #26 and instead introducing a new API without breaking the existing one.

I've uploaded rgain3 1.0.0 to Debian's 'unstable' rolling release, but I'm going to stop it migrating to 'testing' (the alpha version of Debian 11) for the moment.

Sounds good to me.

chaudum commented 4 years ago

In #26 various importable objects were moved around. Was this intentionally an API break? Other packages that import rgain3 as a library, such as https://github.com/rmcauley/rainwave, will presumably need the same changes that rgain3's own tests did.

Given rgain3's rather short history under that name, it's probably OK to be breaking API (and if you want to break API, now is the time!), but it seemed worth checking.

Looking back, I think releasing 1.0.0 was a mistake, since it obstructed the possible introduction of breaking changes with the new package name.

Thanks for bringing that up. I am now thinking of reverting #26 and instead introducing a new API without breaking the existing one.

On the other hand, when changing the upcoming version to 3.0 (as thought in https://github.com/chaudum/rgain/issues/16#issuecomment-630699138) it would "allow" breaking changes, in case there is another 1.x release that deprecates the existing API. What do you think @smcv @rmcauley @paddatrapper ?

I've uploaded rgain3 1.0.0 to Debian's 'unstable' rolling release, but I'm going to stop it migrating to 'testing' (the alpha version of Debian 11) for the moment.

Sounds good to me.

rmcauley commented 4 years ago

I personally use pipenv to avoid this kind of breaking specifically, but this doesn't speak for anyone else. And while I use Debian, because I use pipenv the Debian package versioning does not affect me as I install from source.

I have rgain3 locked to ==1.0.0, so unless I'm affected by a bug, knowing this now I won't be upgrading the library as I have other development priorities. As the program I made using rgain3 is not public-facing, I'm not worried about security concerns, so it'd have to be a bug that breaks my own program that would cause me to upgrade.

For backstory: I needed something to replace mp3gain's dB offset calculation, which itself closely matches my reference foobar2000 replaygain scans. Previously I parsed shell output from mp3gain to get this. To match the same figures, I now have code that looks like this: https://github.com/rmcauley/rainwave/blob/master/libs/replaygain.py It's quite a specific requirement, and was patched in hastily after a Debian upgrade left me without mp3gain. As it was 3 months ago, I don't remember from what point in rgain3's source code I used as a base.

smcv commented 4 years ago

What I would say is: decide what you want the "shape" of the API to be, and do a release, before people other than @rmcauley start relying on it. Breaking API with a major version bump is reasonable from the point of view of semantic versioning, but it's disruptive, because in any given environment (whether that's a pipenv or a distro), import rgain3 can only mean one thing at a time.

Any of these could be viable:

I personally don't see the the scripts' entry points being rgain3.replaygain rather than rgain3.scripts.replaygain as a particularly compelling advantage, and it seems particularly odd for the library code to have moved into a lib subpackage: everything in the import path is library code (whether it's public API or not), so having lib in the import path seems redundant. It's up to you, obviously, but it seems odd to me. Minimizing the typing involved in import rgain3.albumid seems a more important use-case to me than minimizing the typing involved in import rgain3.replaygain, because in practice you'll (presumably) normally be invoking the scripts via their generated entry points like /usr/bin/replaygain?

One thing to beware of when restructuring a Python package is that if you import rgain3.foo.bar, Python will automatically load rgain3 before rgain3.foo and rgain3.foo.bar. This means that if there is anything that is "expensive" to import and will not be needed by all users of the package, you should put it in a "leaf" module rather than at the top level. However, in the restructured version of rgain3, rgain3/__init__.py loads optparse and GStreamer, which both seem like they might fall into the category of things that are expensive to import and/or not always required (particularly since optparse is deprecated in favour of argparse).

For both of those reasons, the old layout (before #26) made more sense to me.

Keeping the scripts' entry points as rgain3.replaygain and rgain3.collectiongain for less typing, but putting everything else back where it was in 1.0.0, would be another possibility.

chaudum commented 4 years ago

Thanks for sharing your thoughts. You have valid arguments, especially with regards to the "expensive imports". They make a lot of sense when rgain3 is used a library. However, personally I've envisioned rgain3 more as a tool, than a library. Therefore the simplification around the scripts' entry points, and moving the "library part" into a the lib module. I would be curious to know how people really use it.

With regards to who to move forward, I tend more and more towards option 2: making a clean cut and release it under 3.x. rgain3 1.0.x could then be seen as the 1to1 Python 3 port of rgain.

smcv commented 4 years ago

Whatever your decision is on this, please could you make the decision and do a release? I've said what I would do if it was up to me, but the specifics of what the decision is matter less than the fact that there is a decision :-)

If you intend rgain3 to be a collection of command-line tools that does not have a stable library interface at all, then I could package it like that in Debian, putting the library part in a private directory that other packages don't import. I personally only use the CLI tools (actually I've mostly switched from replaygain to metaflac/vorbisgain because rgain was Python-2-only, but I'd prefer to be able to return to using one replaygain implementation like rgain3 or https://github.com/Moonbase59/loudgain for multiple formats).

However, if you intend rgain3 to be a library as well, to be used by projects like rainwave, then I need to avoid having incompatible changes happen in Debian as much as possible.

At the moment the API is in limbo (we know the 1.0.0 API is unlikely to stay, but the 3.x API hasn't been in a release) so I've had to exclude rgain3 from consideration for Debian 11.

chaudum commented 4 years ago

Whatever your decision is on this, please could you make the decision and do a release? I've said what I would do if it was up to me, but the specifics of what the decision is matter less than the fact that there is a decision :-)

I understand, uncertainty is bad.

If you intend rgain3 to be a collection of command-line tools that does not have a stable library interface at all, then I could package it like that in Debian, putting the library part in a private directory that other packages don't import. I personally only use the CLI tools (actually I've mostly switched from replaygain to metaflac/vorbisgain because rgain was Python-2-only, but I'd prefer to be able to return to using one replaygain implementation like rgain3 or https://github.com/Moonbase59/loudgain for multiple formats).

I don't think that there will not be a stable API at all, but for the time being there isn't (at least until a 3.0 release when changes are settled).

However, if you intend rgain3 to be a library as well, to be used by projects like rainwave, then I need to avoid having incompatible changes happen in Debian as much as possible.

As already mentioned in the other comment, I envision rgain3 as a collection of tools rather than a library. This may turn people away, but that's how it is.

At the moment the API is in limbo (we know the 1.0.0 API is unlikely to stay, but the 3.x API hasn't been in a release) so I've had to exclude rgain3 from consideration for Debian 11.

Not being available as a Debian package is fine. There will be another 1.x release with official support for Python 3.8 and 3.9 but no other additional features.