mkepub icon indicating copy to clipboard operation
mkepub copied to clipboard

Add optional `ext` keyword argument to `set_cover()`

Open elebow opened this issue 4 years ago • 3 comments

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.

elebow avatar Oct 12 '19 02:10 elebow

Coverage Status

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

coveralls avatar Oct 12 '19 02:10 coveralls

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.

anqxyr avatar Oct 12 '19 07:10 anqxyr

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.

elebow avatar Oct 14 '19 02:10 elebow