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 _format_download_uri_for_extension method #108

Merged
merged 7 commits into from Jun 26, 2018

Conversation

Projects
None yet
2 participants
@c-w
Copy link
Owner

commented Jun 26, 2018

In #105 we saw that there should be a method to leverage the URI building functionality of the Gutenberg library for non-text resources. This pull request implements _format_download_uri_for_extension which is a refactoring of our existing URI formatting logic that can take an arbitrary file extension like -pdf.pdf (as for example returned by the formaturi metadata call) and format the appropriate URL.

This is not an official API (whence the underscore prefix) so no strict requirement to always maintain this function, but it's useful functionality to expose nevertheless given that we already have it in the code base.

When working on this change, I also noticed that we weren't correctly handling the case where the mirror root URL was changed in a single session so I also added a fix for that.

When verifying the refactor, I saw some odd behavior when running the tests locally, so this pull request also improves the developer experience for the tests:

  • We're now using the responses library to stub out the requests.head calls instead of doing it manually.
  • We're now running the tests in a Docker container to ensure that the environment is always known. E.g. for Python 2, BSD-DB should not be installed but it should be installed for Python 3.
  • We're now using python setup.py install to install the dependencies for the various runtimes since the setup file already knows how to load the correct pip dependencies for Python 2 vs Python 3. Previously on Travis we were always installing the Python 3 dependencies even when running the tests for Python 2 which could lead to odd behavior.

c-w added some commits Jun 26, 2018

Ensure GUTENBERG_MIRROR env value gets respected
Currently the GUTENBERG_MIRROR environment variable was only getting
respected for CLI usage, not library usage. This change makes the
handling of the environment variable consistent such that the default
mirror can also be set via an environment variable when the project is
used as a library.
Extract method to format book URI for extension
The `load_etext` function currently throws an exception when there is no
textual download candidate available for a given book. However, some
users might want to use Gutenberg to download non-textual versions of
books. All available formats of a book can already be looked up via the
formaturi metadata extractor, so this change exposes a method to enable
a client to format the download URL for an arbitrary extension.

See #105

@c-w c-w requested a review from hugovk Jun 26, 2018

@hugovk

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2018

python setup.py install to install the correct dependencies is a good idea.

Would it be better to instead call pip install . for the reasons given here?

http://python3statement.org/practicalities/#unit-testing-and-documentation

@c-w

This comment has been minimized.

Copy link
Owner Author

commented Jun 26, 2018

Interesting, didn't know about that. Fixed in ee601ae.

@hugovk

hugovk approved these changes Jun 26, 2018

try:
from functools import lru_cache
except ImportError:
from functools32 import lru_cache

This comment has been minimized.

Copy link
@hugovk

hugovk Jun 26, 2018

Collaborator

Is this due to a Python 2/3 difference?

If so, commenting which is which will make dropping Python 2 easier in the near future.

This comment has been minimized.

Copy link
@c-w

c-w Jun 26, 2018

Author Owner

Yes, functools.lru_cache was only added in Python 3. the functools32 package is a backport for Python 2.7 I'll add a comment.

This comment has been minimized.

Copy link
@c-w

c-w Jun 26, 2018

Author Owner

Done in e49912f.

@c-w c-w merged commit 9f33710 into master Jun 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@c-w c-w deleted the uri-for-extension-function branch Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.