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

Detect and handle CD-R discs #154

Merged
merged 8 commits into from
Jun 6, 2017
Merged

Conversation

gorgobacka
Copy link
Contributor

@gorgobacka gorgobacka commented May 1, 2017

This PR uses cdrdao to detect CD-R discs as discussed in #137 and stops the ripping process with a warning message. The parameter '--cdr' is required to rip this discs (similar to the '--unknown' parameter), indicating that the user should be sure that the inserted medium is reliable.

Additionally, the disc format is added to the output of 'Matching releases' to give the user more information.

Possible enhancements: add this information also to the logger.

@JoeLametta
Copy link
Collaborator

Possible enhancements: add this information also to the logger.

Yep, apart from that it LGTM.

😉

@gorgobacka
Copy link
Contributor Author

Thanks for your comment. I prepared an update.

@gorgobacka
Copy link
Contributor Author

@JoeLametta I would like to know, what do you think about the my update. And I'm wondering, if there are any additional improvements needed before it can be merged.

@JoeLametta
Copy link
Collaborator

I would like to know, what do you think about the my update. And I'm wondering, if there are any additional improvements needed before it can be merged.

I tested it yesterday and the feature seems to be working correctly. Here are the changes I'd like to be implemented before merging:

  1. Rebase (there are conflicts)
  2. PEP8 fix the PR (the whole codebase was recently made compliant)
  3. Remove the MusicBrainz medium format thing (maybe propose it in a separate PR?)

Thanks,
Joe

@gorgobacka
Copy link
Contributor Author

gorgobacka commented Jun 2, 2017

Thanks for your fast reply and the constructive comments. I will provide an update over the weekend.

@@ -143,6 +143,13 @@ def do(self):
"--unknown not passed")
return -1

self.program.result.isCdr = cdrdao.DetectCdr(self.device)
if self.program.result.isCdr and \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better if you break it like this:

        if (self.program.result.isCdr and
                not getattr(self.options, 'cdr', False)):

Copy link
Collaborator

Choose a reason for hiding this comment

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

Everything else is OK

Copy link
Contributor Author

@gorgobacka gorgobacka Jun 5, 2017

Choose a reason for hiding this comment

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

Indeed it looks better. Updated.

@JoeLametta JoeLametta merged commit 4b3d462 into whipper-team:master Jun 6, 2017
@JoeLametta
Copy link
Collaborator

Merged, thanks.

😉

@JoeLametta JoeLametta mentioned this pull request Jan 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants