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

Fixed typo in convert when copying cover art #3063

Merged
merged 5 commits into from
Oct 25, 2018
Merged

Fixed typo in convert when copying cover art #3063

merged 5 commits into from
Oct 25, 2018

Conversation

milesial
Copy link
Contributor

When the convert plugin is copying album arts, it logs

Copying cover art to MY_SOURCE_FOLDER/cover.png

while it should say from (or to the source path). I first I thought my original covers were being overwritten. I fixed the critical issue.

@milesial
Copy link
Contributor Author

I don't think the travis errors come from my commit, I get the same flake errors when i checkout master.

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Awesome; thanks for catching this. You're right that the style errors are not your fault; I've fixed those on master.

I left one comment inline. Also, while I know it seems trivial, would you mind adding a real quick changelog entry describing what you fixed?

util.displayable_path(album.artpath),
util.displayable_path(dest))
self._log.info(u'Copying cover art from {0}',
util.displayable_path(album.artpath))
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason not to include both paths, as in from {0} to {1} here?

Copy link
Contributor Author

@milesial milesial Oct 25, 2018

Choose a reason for hiding this comment

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

Length. In my case it's printing the full path, which is quite long, so two of those would wrap the long line and not be very nice in the console. But as you wish, we can also only log the destination !

Copy link
Member

Choose a reason for hiding this comment

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

I see! But the other messages here include both paths—for example, see the “Resizing” log message a few lines above. It would be nice if all of these were consistent (either way).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you're right, i'm changing it.

@sampsyo
Copy link
Member

sampsyo commented Oct 25, 2018

Looks perfect; thank you! ✨

@sampsyo sampsyo merged commit 146eef3 into beetbox:master Oct 25, 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.

2 participants