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 support for file objects to load_registry() #117

Merged
merged 2 commits into from
Nov 7, 2019

Conversation

remram44
Copy link
Contributor

@remram44 remram44 commented Nov 4, 2019

This makes it possible to pass a file object to load_registry(), opened either in binary or text mode.

My main reason for this is to support passing pkg_resources.resource_stream() to it, which is the preferred way of using data files from the package (it will allow your package to be "zip-safe" though that doesn't matter as much those days).

This could be updated:

GOODBOY.load_registry(os.path.join(os.path.dirname(__file__), "registry.txt"))

to this:

GOODBOY.load_registry(pkg_resources.resource_stream("plumbus", "registry.txt"))

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to the docstring, gallery, and/or tutorials.

pooch/core.py Show resolved Hide resolved
pooch/tests/test_core.py Show resolved Hide resolved
pooch/tests/test_core.py Show resolved Hide resolved
pooch/tests/test_core.py Outdated Show resolved Hide resolved
pooch/tests/test_core.py Show resolved Hide resolved
pooch/tests/test_core.py Show resolved Hide resolved
pooch/tests/test_core.py Show resolved Hide resolved
pooch/tests/test_core.py Outdated Show resolved Hide resolved
pooch/tests/test_core.py Show resolved Hide resolved
pooch/tests/test_core.py Show resolved Hide resolved
@remram44
Copy link
Contributor Author

remram44 commented Nov 4, 2019

Alternatively there could be a separate method that does the load_registry(resource_stream(...)) in one go.

@remram44 remram44 force-pushed the support-fileobj branch 3 times, most recently from 16e31b2 to fa701d9 Compare November 4, 2019 17:22
pooch/tests/test_core.py Outdated Show resolved Hide resolved
pooch/tests/test_core.py Show resolved Hide resolved
pooch/tests/test_core.py Outdated Show resolved Hide resolved
@remram44
Copy link
Contributor Author

remram44 commented Nov 4, 2019

(fixed lint issues: ' -> ", fp -> fin)

pooch/core.py Outdated Show resolved Hide resolved

"""
with open(fname) as fin:
with contextlib.ExitStack() as stack:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, this is new to me. Bookmarking for later use 🙂

Copy link
Member

@leouieda leouieda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this, Rémi 👍 We can add the pkg_resources line to the docs but it might be better to include it as an alternative or recommended way. It's not very straight forward for those not familiar with setuptools (I didn't know about resource_stream) and this is a good chance to introduce it.

Co-Authored-By: Leonardo Uieda <[email protected]>
@remram44
Copy link
Contributor Author

remram44 commented Nov 5, 2019

We can add the pkg_resources line to the docs but it might be better to include it as an alternative or recommended way.

Are you asking for it to be alternative or recommended?

To be honest it is pretty weird to not use pkg_resources in this context, since the example shows creating a complete package, with a setup.py and package_data. There are also multiple ways in which the os.path code can fail, for example if you move it to a sub-package, it will break unless you update it with an extra dirname() (or if running from a ZIP, os.path will construct paths inside the ZIP).

@leouieda
Copy link
Member

leouieda commented Nov 7, 2019

Are you asking for it to be alternative or recommended?

Sorry, my comment wasn't clear. We should definitely recommend this as the preferred approach since it's the correct thing to do (we should probably do it internally as well :). But we should probably acknowledge that the other approach exists and why it's best to do it this way. This can be done in another PR as it's not directly related to this.

Merging this one in since it seems to be complete and all tests pass. Thanks! I always learn some new things from your PRs 🙂

@leouieda leouieda merged commit 31f1bee into fatiando:master Nov 7, 2019
@remram44 remram44 deleted the support-fileobj branch November 7, 2019 17:07
@remram44 remram44 mentioned this pull request Nov 7, 2019
1 task
leouieda pushed a commit that referenced this pull request Nov 18, 2019
Follow up on #117 and document using pkg_resources instead of os.path.
It's less error-prone and allows one's package to work from ZIPs (zip-safe).
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