search

EDyO / appu

Automatic Podcast Publisher
MIT License
3 stars 2 forks source link

Fix image #16

Closed dacacioa closed 5 years ago

dacacioa commented 6 years ago

If you set an incorrect file name to use it as a cover image, appu execution fails at the end. I set a check with a more descriptive error at the beginning of the process. Fixes #14

ifosch commented 6 years ago

Regarding the code, it is only checking the file exists... not sure if that's much useful than having an exception being raised. If that's ok, it's ok to me. However, before merging this, please, clean the PR merge commits. I think this is because this needs a rebase.

dacacioa commented 6 years ago

IMHO is more "elegant" raise an Error instead of a ffmpeg syscall exception.

ifosch commented 6 years ago

Might be more elegant, but it's more processing overhead, not providing anything different, and making debugging harder. I would wait to have appu more mature and used before making it elegant, but, as I already said, if that's ok to you, it's ok to me :)

What I think must be fixed is the amount of commits in this PR. I would suggest to rebase them all squashing on one single one, and then push --force-with-lease them. That way the repository history would be kept cleaner.

ifosch commented 6 years ago

I might not been really going to the point with my review on this. Let me hand this link to reflect that. It might be better to just let the error arise, and then, handle it. More reasoning on this.

ifosch commented 6 years ago

BTW, sorry for being unclear in my review, and for bringing this back again

dacacioa commented 5 years ago

I might not been really going to the point with my review on this. Let me hand this link to reflect that. It might be better to just let the error arise, and then, handle it. More reasoning on this.

Excellent articles and documentation. I didn't know it (referring to EAFP ). I'll adapt the code following this filosophy and I'll have in mind for future developments.