Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix ripping discs with less than ten tracks #418

Merged
merged 1 commit into from
Oct 21, 2019

Conversation

mtdcr
Copy link
Contributor

@mtdcr mtdcr commented Oct 20, 2019

Output lines of cdrdao for single digit track numbers start with a whitespace character. This problem existed since #345 was merged.

Output lines of cdrdao for single digit track numbers start with a
whitespace character.

Broken since whipper-team#345 was merged.

Signed-off-by: Andreas Oberritter <obi@saftware.de>
@JoeLametta
Copy link
Collaborator

Thanks for the pull request!
What about this:

@srussel in #374 (comment)

I also assume the [0-9]* should be [0-9]+ otherwise float() could also fail with 0 digits matched.

P.S.: It's not required but [ ] can be replaced with \s.

@mtdcr
Copy link
Contributor Author

mtdcr commented Oct 21, 2019

@srussel in #374 (comment)

I also assume the [0-9]* should be [0-9]+ otherwise float() could also fail with 0 digits matched.

Sorry, I hadn't noticed #374. I agree, this is a problem that needs fixing. But I just noticed that there are about a dozen occurences of [0-9]* in nearby regular expressions. We probably should replace all of them, but I can't test it right now. Maybe you can merge this patch first and I'll try to send a new PR covering the regular expressions soon.

P.S.: It's not required but [ ] can be replaced with \s.

\s actually matches all kinds of whitespace like \r\n\t etc. Therefore I'd prefer to keep [ ].

@JoeLametta JoeLametta merged commit 8b6b2d3 into whipper-team:develop Oct 21, 2019
@JoeLametta
Copy link
Collaborator

Maybe you can merge this patch first and I'll try to send a new PR covering the regular expressions soon.

👌

@jtl999
Copy link
Contributor

jtl999 commented Oct 21, 2019

Whoops. Pretty sure all disks I tested for #345 at the time had more then ten tracks, so this issue wasn't apparent.

Would've been better if I could've found such a disc [with less then ten tracks] or managed to "simulate" the output from ripping such a disc for testing.

Nice work.

@MerlijnWajer
Copy link
Collaborator

@JoeLametta - I know this issue is closed, but I wonder if we can leverage cdemu to perform automated integration tests of actual rips. I've been doing this for my own CD project, and I must say I'm in love with cdemu. https://cdemu.sourceforge.io

@JoeLametta
Copy link
Collaborator

I know this issue is closed, but I wonder if we can leverage cdemu to perform automated integration tests of actual rips. I've been doing this for my own CD project, and I must say I'm in love with cdemu. https://cdemu.sourceforge.io

That's a good idea. As we're using a privileged Travis CI container I think it should be possible (but need to find the exact configuration).
Of course we should use free tracks like the ones included in this test image. 😉

@mtdcr mtdcr deleted the fix-less-than-ten branch October 30, 2019 20:20
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants