search

rlishtaba / rubyripper

Automatically exported from code.google.com/p/rubyripper
0 stars 0 forks source link

rubyripper should support cdcontrol #485

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago 
FreeBSD comes with cdcontrol by default, which provides output which may be 
used like cd-info without installing any additional ports.  rubyripper should 
make use of cdcontrol if it is present.

I have attached two git patches from format-patch (including rspec tests) 
against rev a8c39b0 to support this.  (git am should suffice to apply the 
patches)

Original issue reported on code.google.com by comradec...@gmail.com on 30 Oct 2011 at 1:18

Attachments:

GoogleCodeExporter commented 8 years ago 
Nice shot, I especially like the rspecs that come with it.

I have two remarks though before I merge it:
* you should state your own name in the copyright thing for the new files. That 
is, unless you want to give me credit for it :)
* In the FreedbString->manualCalcFreedb function you no longer fall back to 
Cdparanoia for disc info. I like to see this fixed, maybe add after the each 
loop: @scan = @disc if @scan.nil? (Perhaps a spec for this is missing, oops ;))

I can't really test if the output in the spec is actually the output of 
Cdcontrol, but I suppose you've tested this yourself ;)

For the long run the code is getting a bit messy. I'd like to fire the Disc 
class from the obligation to pass these objects as well as the FreedbString 
class. But these concerns can be solved later on now we got specs ;)

Original comment by boukewou...@gmail.com on 30 Oct 2011 at 8:36

GoogleCodeExporter commented 8 years ago 
I read your patch wrong, so the second remark is wrong. That only leaves the 
first one. Let me know what to do about it ;)

Meanwhile I've merged your patch. I'll see if I can cleanup the structure 
somewhat.

Original comment by boukewou...@gmail.com on 30 Oct 2011 at 9:17

GoogleCodeExporter commented 8 years ago 
With latest commit I've refactored the code a bit. Beautifull once again :)

Original comment by boukewou...@gmail.com on 30 Oct 2011 at 12:03

GoogleCodeExporter commented 8 years ago 
Yeah, I'll go ahead and add my name for the copyright in future patches (in 
fact, I'm going to be submitting a rather large set to another feature request 
shortly).

Original comment by comradec...@gmail.com on 31 Oct 2011 at 6:33

GoogleCodeExporter commented 8 years ago 
In the meanwhile, however, your refactoring broke a few things:

1. ScanDiscCdcontrol is broken as it expects an Execute argument which is no 
longer being explicitly specified.  This is patched in 
0001-Fix-scanDiscControl-arguments.patch, attached.
2. FreedbString is broken as the scan() method is no longer being called on the 
TOC scanner that is received from the Disc object.  This is patched in 
0002-Call-scan-in-FreedbString.patch, attached

(Again, git am should suffice to import the fixes.)

Also, I've added the attribution patch in 
0003-Fix-copyright-attribution-in-scanDiscCdcontrol.rb.patch

Original comment by comradec...@gmail.com on 31 Oct 2011 at 6:43

Attachments:

GoogleCodeExporter commented 8 years ago 
Oop, forgot to fix copyright in the rspec file.  Attached.

Original comment by comradec...@gmail.com on 31 Oct 2011 at 6:45

Attachments:

GoogleCodeExporter commented 8 years ago 
Yup, you're right. I've merged above patches. Thanks for keeping them small, 
makes it real easy to merge :D

Original comment by boukewou...@gmail.com on 31 Oct 2011 at 7:19