Add ability to skip tracks #260
Conversation
Shorten one line for flake8 compliance
|
I tested this patch successful on older whipper 0.5.1 commit 1247c94 + 88c7abc using the following command: This will successfully rip track 1&14 of a 14 track CD. I haven't tested CD's with more than 100th tracks but its easy to work with, although the LOG indeed says excluded rips are done 100%. However if I use current git master commit db8050d whipper version 0.7.0, it seems that skipping/excluding the first track will have difficulty reading only the last 14th track and fails. Giving offset /zero/checksum errors. I'll try to upload those log errors later. Tested using two different drives Samsung usb and HL LG sata each verified different offset +6/+667. |
| @@ -455,7 +461,16 @@ def _ripIfNotRipped(number): | |||
| self.ittoc.getTrackLength(number), number) | |||
|
|
|||
| self.program.saveRipResult() | |||
|
|
|||
| try: | |||
There was a problem hiding this comment.
I think it'd be preferable to just check whether the option is the empty string or not if you were going to do it this way. See further comments.
| try: | ||
| excluded_tracks_integer.append(int(i.replace(" ", ""))) | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
Instead of silently ignoring errors, we should abort at argument-handling time with a helpful yet short error message.
| excluded_tracks = self.options.excluded_tracks.split(",") | ||
| except UnboundLocalError: | ||
| excluded_tracks = [] | ||
| excluded_tracks_integer = [] |
There was a problem hiding this comment.
I think we should keep the array of excluded tracks in options.excluded_tracks.
| action="store", dest="excluded_tracks", | ||
| default="", | ||
| help="Comma separated list of tracks to " | ||
| "exclude. This will propably mess up " |
There was a problem hiding this comment.
I think it'd be best to handle the logging situation as part of implementing this feature. We'd like to avoid having incompatible features butting heads.
| except UnboundLocalError: | ||
| excluded_tracks = [] | ||
| excluded_tracks_integer = [] | ||
| for i in excluded_tracks: |
There was a problem hiding this comment.
I think it would be better to parse the track listing in the handle_arguments() method. I also bet there's a way to tell argparse to expect a variable number of integers, which will save us from having to manually typecast strings to integers.
|
Thanks for the contribution. I've left a few comments I think should be addressed before we continue. I'll need input from the rest of the team on whether we want to implement this feature or not. Another option would be to implement a "rip only these tracks" feature, something I see as a little easier to use than a blacklist. @walterav1984 thanks for testing. Do either of you think you'd be able to contribute unit tests of this functionality? |
|
What would you mean by unit tests? |
|
I can confirm that indeed the comparisons of the checksums will fail (mismatch) since it uses the hash of the previous/skipped n-th track. Sorry for not providing a descent log, will try in future to reproduce. |
|
No activity for a long time: closed. |
It works, but is propably not using the best way to do this