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

put() takes parameters in a weird format that now isn't even given by discid #29

Merged
merged 4 commits into from
Apr 26, 2013

Conversation

JonnyJD
Copy link
Owner

@JonnyJD JonnyJD commented Apr 25, 2013

The offsets parameter wants the first item to be the number of sectors in the disc and the rest of the list to be track offsets.
We should split that to two parameters, especially now, since we don't provide a similar list anymore.

@JonnyJD
Copy link
Owner Author

JonnyJD commented Apr 25, 2013

This is of course related to the changes introduced for #24 and #27.

The downside, if we change this, is that our parameters are different then the ones libdiscid expects directly. We are not the only binding changing that though.

@ghost ghost assigned JonnyJD Apr 25, 2013
We should at least check against common usage errors
This is not actually an IOError, in contrast to read().
put gives errors on incorrect usage/parameters.
@JonnyJD
Copy link
Owner Author

JonnyJD commented Apr 26, 2013

I want to note, that I don't want to remove either of the parameters first and last. While technically possible, since that can be guessed from the length of the offsets, I want them as another sanity check to make sure that we don't get bogus disc IDs.
The internal check is done in 08d1cde. Since there are different methods of handing over the sector count and the offsets, I check the length against the track numbers.

@phw
Copy link

phw commented Apr 26, 2013

The difference between output and input is a point that should be considered. rb-discid has that issue, too. Maybe there should be an offsets property on the disc object with the correct format? What do you think?

I would also definitely keep the parameter first. For last rb-discid makes the assumption that it can be calculated from first and the offset length, which should be fine since the numbering is consecutive. However having it as an explicit parameter is probably more clear to the user and allows for extended input validation. Either way is fine.

JonnyJD added a commit that referenced this pull request Apr 26, 2013
See pull-request #29

Conflicts:
	test_discid.py
@JonnyJD JonnyJD merged commit c19b4f3 into master Apr 26, 2013
@JonnyJD
Copy link
Owner Author

JonnyJD commented Apr 26, 2013

@phw:
The Disc.offsets property goes into the direction of http://tickets.musicbrainz.org/browse/LIB-41 (TOC "id").
I am thinking about implementing Disc.toc in python-only (so it doesn't depend on a new libdiscid version).

The difference between Disc.toc and Disc.offsets is:
The toc can be used as an input for a MusicBrainz query. However, except for internal testing, there is no good reason to use python-discid to generate Disc.offsets just to then use this as an input for put().

@JonnyJD JonnyJD deleted the simple_put branch April 26, 2013 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants