-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Added option to embedart using an image URL [small PR] #4719
Conversation
Just had a quick look: Maybe you could add error handling for:
Best you look around how others do stuff like that. E.g file handling in the smartplaylist plugin for an example and maybe fetchart itself for requests errors catching. |
Some example of handling file errors and also important: How to make use of syspath() normpath, bytestring_path functions, which is an importent aspect of how beets handles files: https://github.com/beetbox/beets/pull/4719/files#diff-5a7d57cfa16b2afc519d39be035fd1808000f256b044d8def53142c97e46196eR95-R101 I'm also only learning stuff like that with beets, so hope that is the right pointer. Recently I worked on this plugin and copied how files are handled there: beets/beetsplug/smartplaylist.py Lines 221 to 234 in ed1202b
Well, actually errors are _not_handled here. In my opinion the with block needs to be wrapped into try/except and then an OSError should be catched and error class the utils package provides should be thrown. I think like that is making sure a "nice" error is displayed and no ugly exception (the FileSystemError you can import from beets.utils package) https://github.com/beetbox/beets/pull/4399/files#diff-f20a2ab41ac3a3c83575cac2b871653999b0e4d28c27336a1f3897a004f5d6daR86-R93 |
@JOJ0 I have added error handling. I think the path-related comment is less relevant, as the file is saved in the temp folder and removed later, i.e., the path is less important here. |
Nice! You could use the exception provided in utils for the file handling. Maybe beets has one provided for requests/apis as well. Dont know.. |
But not sure if it works with Image.open |
Ok I see you did something similar to what we have in fetchart, so I guess that's fine. Lines 370 to 376 in ed1202b
|
beetsplug/embedart.py
Outdated
with open('temp.jpg', 'wb') as f: | ||
f.write(response.content) | ||
except Exception as e: | ||
self._log.error(f"Error writing file: {e}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here a few examples of how to use the file error exception class beets already provides: https://github.com/search?q=repo%3Abeetbox%2Fbeets%20FileSystemError&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 534 to 536 in ed1202b
except OSError as exc: | |
raise FilesystemError(exc, 'link', (path, dest), | |
traceback.format_exc()) |
This for example will result in a nicely human readable message like: "Error durint linking of paths ....."
Instead of the word "link" you could use "write" in your case. The error class will handle the rewording magic! Give it a try! It's a cool concept in beets. We don't have to reinvent the wheel :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the definition of what will happen when "write" is given exactly:
Lines 106 to 124 in ed1202b
def get_message(self): | |
# Use a nicer English phrasing for some specific verbs. | |
if self.verb in ('move', 'copy', 'rename'): | |
clause = 'while {} {} to {}'.format( | |
self._gerund(), | |
displayable_path(self.paths[0]), | |
displayable_path(self.paths[1]) | |
) | |
elif self.verb in ('delete', 'write', 'create', 'read'): | |
clause = 'while {} {}'.format( | |
self._gerund(), | |
displayable_path(self.paths[0]) | |
) | |
else: | |
clause = 'during {} of paths {}'.format( | |
self.verb, ', '.join(displayable_path(p) for p in self.paths) | |
) | |
return f'{self._reasonstr()} {clause}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the approach used in fetchart
and playlist
plugins.
Lines 352 to 353 in ed1202b
except util.FilesystemError as exc: | |
self._log.debug('error cleaning up tmp art: {}', exc) |
I feel that the following may needlessly complicate things as we will have to define
path
and dest
, which may not be needed here.
except OSError as exc:
raise util.FilesystemError(exc, 'write', (path, dest),
traceback.format_exc())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace path+dest with one thing then
see the definition I posted: #4719 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you misunderstood: You should still catch oserror but then raise FilesystemError
Filesystemerror will translate your exception into human readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
caught OSError. let's hope this works.
Looks like tests are failing for unrelated reasons... |
I realised I can simply re-run failed tests. I think it was just some problems with the underlying cloud servers at github/ms/whatever. Rerun worked. I think you left a debug print around that tempimg code. Maybe post some test runs with good and bad input values. I won't get to testing myself.... |
@JOJ0 Removed the debug print. All the test runs work fine:
|
@arsaboo sorry for the delay. I realised embed art is pretty well covered with unittests and it would make sense to add some for this new feature. I just pushed a test that checks for proper fetching and embedding of a jpeg file to your feature branch. I hope that is ok. I know you once told me that writing tests is not in your bag of tricks yet. Here I'd like to encourage you to give it a try. You could copy A third copy of this test could test if we properly fail when no image is fetched (with something other than jpeg or png, the test helper would return nothing I think, and we'd get an invalid url error). We could test then if the media file has no image embeded. hope that helps and get back to me with questions. Also @sampsyo or @Serene-Arc could you have a quick look if my test makes sense that way or if I'm not testing anything worthwhile. Just to make sure. Thanks! |
@JOJ0 Thanks for the review and for adding a test. I added a few tests. Hopefully, this should be enough to get this merged. As I mentioned earlier, tests are not something I am very comfortable with, and would rather not mess with them unless it is an ABSOLUTE MUST to get this PR merged. Feel free to modify the PR to add anything that you feel will improve the code quality. |
and shorten second and third test a little by providing -y cli flag. Enough to test with interactive input once. Move all 3 tests to the very bottom of the test class.
Oh, it should not feel like a burden. I just thought it makes sense and actually is a rather easy opportunity to learn something about tests. The beauty of beets is that almost everything that could be tested is found around the codebase somewhere. Big opportunities to learn about testing everywhere. Also the nice thing about tests is, it makes you see thing you wouldn't even with damn thorough manual test runs. I just realised that actually we could download html files and try to embed, but in that case the error handling of embed to file function itself kicks in and handles it. So I think that's good enough:
In a real world test this looks like this:
|
Makes it more clear what we are doing.
Oh I see you mentioned above that this is actually expected: #4719 (comment) I needed a test to read your comment haha ;-) Thanks a lot for the patience and all! We went from almost no error handling to pretty bulletproof error handling and even have some unittesting. So, yeah, I think it was all worth it. Thanks again! Great! Now I can fix my sometimes funny random album art ;-) Merging... |
Thanks @JOJ0 for your patience and for guiding me through this. BTW, where/how are you running |
Simply run it in the terminal. From the root of the repo. In vscode terminal or regular terminal. |
Description
Fixes #83
Added another option
-u
to provide an image URL. The plugin can now download the image, check if it is valid, and then embed the picture as cover art.To Do
docs/
to describe it.)docs/changelog.rst
near the top of the document.)