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

Add optional ext keyword argument to set_cover() #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

elebow
Copy link
Contributor

@elebow elebow commented Oct 12, 2019

Previously, set_cover() would rely on the Python standard module imghdr to determine the type of the specified cover image file. Unfortunately, imghdr does not support SVG, so set_cover() could not handle that type (which is permitted by the EPUB spec).

Detecting SVG files programmatically is nontrivial. Instead, simply provide an option for the client to specify the file type with the new keyword argument.

This introduces a minimal addition to the public API of the module.


I have two reservations about this PR:

  1. The parameter name ext. It's not exactly an extension. But it's not exactly a MIME type, either. I'm not sure what a better name would be.
  2. This PR introduces unit testing to the application. There weren't any examples to follow in the existing tests (only high-level integration tests), so I've used my personal preferences regarding mocking and using a class. I'm certainly open to other approaches.

@coveralls
Copy link

coveralls commented Oct 12, 2019

Coverage Status

Coverage increased (+0.02%) to 98.98% when pulling 2af5d92 on elebow:set-cover-arg-extension into 7dd07d9 on anqxyr:master.

@anqxyr
Copy link
Owner

anqxyr commented Oct 12, 2019

It's not exactly an extension. But it's not exactly a MIME type, either.

It is exactly an extension, since it's used as the extension in the filename for the cover. Although I think that using the full word extension would be more clear for the public API instead of ext.

This PR introduces unit testing to the application.

Not a fan of the unittest lib or class-based tests. mkepub already uses pytest for testing, and it has fixtures that should work just fine with function-based tests. Will rewrite this a bit later before merging.

@elebow
Copy link
Contributor Author

elebow commented Oct 14, 2019

Changed the parameter name to extension.

Changed the tests to use pytest, but also pytest-mock. I'm not sure how you feel about that plugin, but the general approach is about the same as it was before.

Regarding the test class, it's purely for organizational purposes in the test code (and allows, for example, pytest -k TestSetCover). Here's a branch without it, if you prefer that: https://github.com/elebow/mkepub/commits/set-cover-arg-extension-no-test-class. I can update this PR you want.

Previously, set_cover() would rely on the Python standard module
`imghdr` to determine the type of the specified cover image file.
Unfortunately, imghdr does not support SVG, so set_cover() could
not handle that type (which is permitted by the EPUB spec).

Detecting SVG files programmatically is nontrivial, and is generally
considered to require parsing the file source.

Instead, simply provide an option for the client to specify the file
type with the new keyword argument.
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