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

switch CDDB implementation to freedb.py from python-audio-tools #276

Merged
merged 5 commits into from
Jun 8, 2018

Conversation

mtdcr
Copy link
Contributor

@mtdcr mtdcr commented May 13, 2018

Source: https://github.com/tuffy/python-audio-tools/blob/master/audiotools/freedb.py (GPL-2.0+)

The last release of cddb-py was 15 years ago. The freedb code taken from python-audio-tools speaks protocol version 6 (Unicode) and is compatible to both Python 2 and 3.

The first motivation to get rid of cddb-py was its unavailability in pip. Python-audio-tools isn't there either, and its freedb code can't be used as an independent library, but as the source code looks small and clean and comes with a matching license, I guess it's sane to just take the code from there and maintain it inside whipper.

mtdcr added 4 commits May 13, 2018 22:29
The removed functions depend on other classes of python-audio-tools,
but aren't needed here.
The last release of cddb-py was 15 years ago. The freedb code taken
from python-audio-tools speaks protocol version 6 (Unicode) and is
compatible to both Python 2 and 3.
@RecursiveForest
Copy link
Contributor

I suspect it'd be a better long term solution to package freedb.py as its own library (can be used by more than whipper, commits to sync with upstream won't have to show up in our commit history, etc), but barring that this approach is acceptable.

Merging is not contingent on a positive answer, but were people having issues with cddb-py not supporting protocol version 6?

@mtdcr
Copy link
Contributor Author

mtdcr commented May 31, 2018

I don't know of any issues with the old implementation besides its unavailability from pip.

@JoeLametta JoeLametta changed the title RFC: switch CDDB implementation to freedb.py from python-audio-tools switch CDDB implementation to freedb.py from python-audio-tools Jun 8, 2018
@JoeLametta JoeLametta merged commit 542e071 into whipper-team:master Jun 8, 2018
@Freso
Copy link
Member

Freso commented Jun 12, 2018

I commented on upstream, asking about them packaging this for PyPI: tuffy/python-audio-tools#33 (comment)

@mtdcr
Copy link
Contributor Author

mtdcr commented Jun 12, 2018

@Freso: Actually the file imported from python-audio-tools was modified quite a bit in order to remove application logic and dependencies: fdeeed9

@Freso
Copy link
Member

Freso commented Jun 12, 2018

@mtdcr Is there any reason the module couldn't work if imported as from audiotools import freedb? The only interface change seems to be the output of perform_lookup() yielding the raw FreeDB data(?) instead of an xmcd_metadata() processed list? I'm sure the whipper code can be modified to handle this different data.

@mtdcr mtdcr deleted the freedb branch June 12, 2018 18:28
@mtdcr
Copy link
Contributor Author

mtdcr commented Jun 12, 2018

IIRC, xmcd_metadata returns a stripped-down version of the response formatted in a way specific to audiotools.

The module also identifies itself as "audiotools" to cddb servers, which wouldn't be polite if used by whipper.

I'm not arguing against distribution of audiotools via pypi, but I guess whipper's use of freedb.py is not a good argument for it either. freedb.py is just a small part of audiotools.

To let freedb.py be useful for whipper or other projects, I believe that audiotools would have to solve both problems mentioned above, e.g. by creating a second interface to get access to the raw response and by allowing to set a custom application name and version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants