Skip to content

Commit

Permalink
use discid_read_sparse when available
Browse files Browse the repository at this point in the history
We only want to read the TOC when no features are given explicitely.
  • Loading branch information
JonnyJD committed Apr 7, 2013
1 parent 4ab3ae1 commit c998986
Showing 1 changed file with 37 additions and 4 deletions.
41 changes: 37 additions & 4 deletions discid.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
import os
import sys
import ctypes
from ctypes import c_int, c_void_p, c_char_p
from ctypes import c_int, c_void_p, c_char_p, c_uint, sizeof
from ctypes.util import find_library


Expand Down Expand Up @@ -206,6 +206,7 @@ def __init__(self):
"""
self._handle = c_void_p(_LIB.discid_new())
self._success = False
self._requested_features = []
assert self._handle.value is not None

def __str__(self):
Expand All @@ -225,23 +226,55 @@ def _get_error_msg(self):

_LIB.discid_read.argtypes = (c_void_p, c_char_p)
_LIB.discid_read.restype = c_int
def read(self, device=None):
"""Reads the TOC from the device given as string.
try:
_LIB.discid_read_sparse.argtypes = (c_void_p, c_char_p, c_uint)
_LIB.discid_read_sparse.restype = c_int
except AttributeError:
pass
def read(self, device=None, features=[]):
"""Reads the TOC from the device given as string
That string can be either of:
:obj:`str <python:str>`, :obj:`unicode` or :obj:`bytes`.
However, it should in no case contain non-ASCII characters.
If no device is given, the :data:`DEFAULT_DEVICE` is used.
You can optionally add a subset of the features in
:data:`FEATURES` or the whole list to read more than just the TOC.
In contrast to libdiscid, :func:`read` won't read any
of the additional features by default.
A :exc:`DiscError` exception is raised when the reading fails,
and :exc:`NotImplementedError` when libdiscid doesn't support
reading discs on the current platform.
"""
if "read" not in FEATURES:
raise NotImplementedError("discid_read not implemented on platform")

python_discid_features = ["read", "mcn", "isrc"]
# only use features implemented on this platform and in this module
self._requested_features = list(set(features) & set(FEATURES)
& set(python_discid_features))

This comment has been minimized.

Copy link
@phw

phw Apr 7, 2013

Due to the way discid_read_sparse is working, this is not strictly necessary, as unsupported features are supposed to be ignored. Not an error, though. But I personally tend to reduce code complexity in cases like this.

This comment has been minimized.

Copy link
@JonnyJD

JonnyJD Apr 8, 2013

Author Owner

This is necessary when I want to know if the feature was not requested (because the module doesn't know how) or there is just no result for this feature. (difference between "" and None in the feature "getter").


# create the bitmask for libdiscid
if set(features) == set(FEATURES):

This comment has been minimized.

Copy link
@phw

phw Apr 7, 2013

This behavior somewhat contradicts the check for supported features above. You enable all features (even those not in python_discid_features) if the user requests all features supported by the platform. That means if the platform does support the "my_new_feature" feature, but you have not yet added support for this to python-discid, the user can not enable it by passing ["my_new_feature"]. But he can enable it if he passes all features supported by the platform, e.g. ["read", "mcn", "isrc", "my_new_feature"]. Another reason to get rid of the check above :D

This comment has been minimized.

Copy link
@JonnyJD

JonnyJD Apr 8, 2013

Author Owner

Hm, I should remove the & set(python_discid_features) in that case.

This comment has been minimized.

Copy link
@phw

phw Apr 8, 2013

After looking at it again I think you should rather remove this whole "if set(features) == set(FEATURES)" block. What was the intention here? IMHO it just is an attempt to be clever (the user requested all features, let's re-write it to UINT_MAX). This is not necessary, as the code in the else path will just lead to the same result for all supported features. But it ads some hidden functionality, e.g. ["read", "mcn", "isrc", "my_new_feature"] will use "my_new_feature" but ["read", "mcn", "my_new_feature"] won't.

The only purpose I can see is, that it adds a way to set features not yet supported by python-discid. But if that was the intention it is the wrong way to solve it. The main problem would be that libdiscid has only partial support for the feature strings and does not expose a way to map a feature string to the enum value, which forces python-discid to include such a mapping and update it for every feature.

This comment has been minimized.

Copy link
@JonnyJD

JonnyJD Apr 8, 2013

Author Owner

When writing this I was thinking about whether "isrc_raw" and "isrc_cdtext" should be features in terms of the feature API or not.
If they are, the bindings need updates to include these features (or some way to work around this).
What I tried here is "a way around this", but not at all a nice one.

We probably shouldn't expose "isrc_raw" and "isrc_cdtext" as features for this reason. On the other hand it won't be possible for the user to decide if reading ISRCs from CD-TEXT (or raw) should be done or not.

A mapping from a feature string to a bit mask value is a possibility, or the other way around (would be easier in libdiscid, but the users would have to check all possible bits (32)).
Not really sure if that is necessary. This would only make sense for new features not yet known by the binding.

This comment has been minimized.

Copy link
@phw

phw Apr 8, 2013

I think it is acceptable to have the mappings in the bindings. I don't expect much features added to libdiscid, and it is likely that a new feature will require changes to the bindings other than updating the mapping. So it's ok to have the mapping in the binding and do a new release for new features. Therefore I would not worry about unsupported features here, just use the default "else" path and update the mapping if features get added.

This comment has been minimized.

Copy link
@JonnyJD

JonnyJD Apr 8, 2013

Author Owner

You got me convinced and that UINT_MAX stuff is removed in 29085a4.

# full feature set requested
# possibly some features can't be accessed by this module
c_features = 2 ** (8 * sizeof(c_uint)) - 1 # UINT_MAX
else:
c_features = 0
for feature in features:
if feature == "mcn":
c_features += 1 << 1

This comment has been minimized.

Copy link
@phw

phw Apr 7, 2013

Do you have access to the actual enum values with ctypes? Would be cleaner than using the magic numbers here (or add some constants).

This comment has been minimized.

Copy link
@JonnyJD

JonnyJD Apr 8, 2013

Author Owner

Nope, no access to the actual values. Doesn't make a logical difference though. When a new value is added the code needs a change. With or without having enums available directly. (changing values is something we don't want to do with libdiscid)
Constants are an option in general, but I dislike creating a block of constants for a block of code filled with constants. That bloats the code and doesn't make things any easier or easier to understand. Unless of course the constants are used in multiple places.
Technically the string defines could be of help to the user, but these should be taken from the API documentation I'd say and people are not supposed to write read(features=[FEATURE_ISRC, FEATURE_MCN]) rather than read(features=["isrc", "mcn"]).

I should however state where these numbers come from in a comment.

Although I realize: I still have documented the features as ["read", "MCN", "ISRC"]. So I should try to create the documentation from constants maybe..

This comment has been minimized.

Copy link
@JonnyJD

JonnyJD Apr 8, 2013

Author Owner

I created a dict that basically implements the enum in python, only better (as a dictionary) in 8697d8c.

That is actually a lot cleaner. When answering here I was thinking of 3-6 constants, which would be bloat, but a dict is fine. Thanks for your feedback.

This comment has been minimized.

Copy link
@phw

phw Apr 8, 2013

Beautiful :)

if feature == "isrc":
c_features += 1 << 2

# device = None will use the default device (internally)
result = _LIB.discid_read(self._handle, _encode(device)) == 1
try:
result = _LIB.discid_read_sparse(self._handle, _encode(device),
c_features) == 1
except AttributeError:
result = _LIB.discid_read(self._handle, _encode(device)) == 1

This comment has been minimized.

Copy link
@phw

phw Apr 7, 2013

It would be better to detect the existence of discid_read_sparse once instead of relying on exceptions on each call to read, It's not much of a performance issue in this case (there is no real use case of calling read multiple times in a performance critical situation), but since you actually test for read_sparse once in line 229 there is no need to duplicate that check here. I would just set some constant above and check it here.

This comment has been minimized.

Copy link
@JonnyJD

JonnyJD Apr 8, 2013

Author Owner

Actually try-except is the standard way of dealing with things that should just work under normal conditions and actually faster than using a check up-ahead. Exceptions are not a bad thing in python:
http://docs.python.org/2/howto/doanddont.html#exceptions

I'd say having read_sparse is the normal condition since python-discid is not even in any repository yet and many users of the module will soon have libdiscid 0.5.0. Even more so in all the years this code will stay unchanged ;-)

I could set _read_sparse = True up ahead, but that doesn't make anything easier, shorter or faster.

self._success = result
if not self._success:
raise DiscError(self._get_error_msg())
Expand Down

0 comments on commit c998986

Please sign in to comment.