Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
smcv opened this issue Aug 10, 2020 · 7 comments
Open

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

smcv opened this issue Aug 10, 2020 · 7 comments
Labels
discuss This issue or pull request needs further discussion roadmap Questions and discussions around the roadmap

Comments

@smcv
Copy link
Contributor

smcv commented Aug 10, 2020

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
Copy link
Owner

chaudum commented Sep 3, 2020

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
Copy link
Owner

chaudum commented Sep 3, 2020

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 #16 (comment)) 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.

@chaudum chaudum added the discuss This issue or pull request needs further discussion label Sep 3, 2020
@rmcauley
Copy link

rmcauley commented Sep 4, 2020

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
Copy link
Contributor Author

smcv commented Sep 4, 2020

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:

  • Go back to a 1.0.0-compatible API. Release a new version.
  • Make a clean break and go to a new API. Release a new version 3.x.
  • Make the API compatible with both 1.0.0 and the new "shape" you prefer, with the 1.0.0 API issuing deprecation warnings. Release a new version in that state. Eventually bump the major version and drop the 1.0.0-compatible API.

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
Copy link
Owner

chaudum commented Sep 4, 2020

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.

@chaudum chaudum added the roadmap Questions and discussions around the roadmap label Sep 4, 2020
@smcv
Copy link
Contributor Author

smcv commented Sep 30, 2020

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
Copy link
Owner

chaudum commented Sep 30, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss This issue or pull request needs further discussion roadmap Questions and discussions around the roadmap
Projects
None yet
Development

No branches or pull requests

3 participants