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.

Sign up for GitHub

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

Jump to bottom

Add ability to skip tracks #260

Closed
wants to merge 4 commits into from
Closed

Add ability to skip tracks #260

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1e907e2
Add ability to skip tracks
waweic Apr 8, 2018
aa21395
Update cd.py
waweic Apr 8, 2018
5084801
Merge branch 'master' into master
waweic May 23, 2018
06c02b6
Merge branch 'master' into master
waweic Aug 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion whipper/command/cd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,12 @@ def add_arguments(self):
"if the patched cdparanoia package is "
"installed and the drive "
"supports this feature. ")
self.parser.add_argument('--exclude-tracks',
action="store", dest="excluded_tracks",
default="",
help="Comma separated list of tracks to "
"exclude. This will propably mess up "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"your logs")
self.parser.add_argument('-O', '--output-directory',
action="store", dest="output_directory",
default=os.path.relpath(os.getcwd()),
Expand Down Expand Up @@ -455,7 +461,16 @@ def _ripIfNotRipped(number):
self.ittoc.getTrackLength(number), number)

self.program.saveRipResult()

try:
Copy link
Contributor

@RecursiveForest RecursiveForest Jun 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

excluded_tracks = self.options.excluded_tracks.split(",")
except UnboundLocalError:
excluded_tracks = []
excluded_tracks_integer = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the array of excluded tracks in options.excluded_tracks.

for i in excluded_tracks:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try:
excluded_tracks_integer.append(int(i.replace(" ", "")))
except ValueError:
pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of silently ignoring errors, we should abort at argument-handling time with a helpful yet short error message.

# check for hidden track one audio
htoa = self.program.getHTOA()
if htoa:
Expand All @@ -471,6 +486,9 @@ def _ripIfNotRipped(number):
# FIXME: make it work for now
track.indexes[1].relative = 0
continue
if i + 1 in excluded_tracks_integer:
print("excluding track {}".format(i + 1))
continue
_ripIfNotRipped(i + 1)

logger.debug('writing cue file for %r', discName)
Expand Down