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

Fix image #16

Closed
wants to merge 4 commits into from
Closed

Fix image #16

wants to merge 4 commits into from

Conversation

dacacioa
Copy link
Member

@dacacioa dacacioa commented Oct 1, 2018

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
Copy link
Member

ifosch commented Oct 5, 2018

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
Copy link
Member Author

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

@ifosch
Copy link
Member

ifosch commented Oct 18, 2018

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
Copy link
Member

ifosch commented Oct 18, 2018

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
Copy link
Member

ifosch commented Oct 18, 2018

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

@dacacioa
Copy link
Member Author

dacacioa commented Jan 3, 2019

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.

@dacacioa dacacioa closed this Jul 12, 2019
@dacacioa dacacioa deleted the fix-image branch July 12, 2019 13:19
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.

Check cover image
2 participants