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

Initial implementation of pystac_io #1

Merged
merged 8 commits into from
Oct 30, 2020
Merged

Initial implementation of pystac_io #1

merged 8 commits into from
Oct 30, 2020

Conversation

CloudNiner
Copy link
Contributor

@CloudNiner CloudNiner commented Oct 28, 2020

Overview

See the new README.md in this PR for installation, usage, and development instructions.

Soliciting feedback for the architecture and API provided by these modules:

  • Easy to understand?
  • Easy to setup?
  • Easy to implement and use new module?
  • Missing features?

Changes:

  • Installation, Usage, and Development
    documentation in README
  • Sample code in README
  • Linting with flake8
  • Formatting with black
  • scripts/test runs linting and formatting
  • Code to initialize and load
    pystac_io submodules
  • https module
  • s3 module
  • Apache 2.0 License
  • setup.py for local pip installation
  • requirements-dev.txt for consistent dev env
  • Basic GitHub Actions CI from Add GitHub Actions CI runner #2

Demo

Screen Shot 2020-10-28 at 2 06 35 PM

- Installation, Usage, and Development
  documentation in README
- Sample code in README
- Linting with flake8
- Formatting with black
- scripts/test runs linting and formatting
- Code to initialize and load
  pystac_io submodules
- `https` module
- `s3` module
- Apache 2.0 License
- setup.py for local pip installation
- requirements-dev.txt for consistent dev env
Only runs code formatter and linter
for now -- test suite coming soon.
Add GitHub Actions CI runner
import pystac_io
import pystac_io.s3 # Import the IO module you plan to `register()`

pystac_io.register("s3")
Copy link
Contributor Author

@CloudNiner CloudNiner Oct 28, 2020

Choose a reason for hiding this comment

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

The primary alternative I considered here was to just expose all of the underlying {module}_[read|write]_text_method functions and it's up to the user to manually apply them to pystac.STAC_IO. That would look something like:

import pystac
from pystac_io.s3 import s3_read_text_method, s3_write_text_method

pystac.STAC_IO.read_text_method = s3_read_text_method
pystac.STAC_IO.write_text_method = s3_write_text_method

s3_catalog = pystac.Catalog.from_file("s3://bucket/path/to/catalog.json")
s3_catalog.normalize_and_save(...)

# Revert to defaults
pystac.STAC_IO.read_text_method = pystac.STAC_IO.default_read_text_method
pystac.STAC_IO.write_text_method = pystac.STAC_IO.default_write_text_method

If you just want a read method and don't need to swap to different modules, its about the same code either way. If you need read+write and are swapping modules, the extra wrapper methods implemented in this PR reduce boilerplate. They also provide a simple interface that only requires one extra line of code for users to define and use their own modules.

I implemented the wrapper API in this PR because it was the "harder" option and seemed useful to demonstrate what it ended up looking like. If it's too complicated for what's essentially a lose collection of methods and their dependencies I don't have any major concern with reverting to the version described here without the wrapper API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd lean towards a wrapper API that implements a context manager in light of #1 (comment)

README.md Outdated

#### Multiple IO Modules

`pystac.STAC_IO` is only able to register a single global read and write handler. If you need to use multiple `pystac_io` modules in the same script you need to unregister one before registering another:

Choose a reason for hiding this comment

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

This is screaming out for a context manager. I've never implemented one -- do you know if it's tricky?

with pystac_io.register("s3"):
    ...

seems like a much better idiom than requiring people to remember to unregister things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🔥 that's a great idea. Context managers are pretty straightforward, take a look at the Examples section of PEP343. Theres a function decorator so you don't have to construct a class with __enter__ and __exit__ methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up being a bit trickier than expected to get pystac_io.register to work as a regular function and be a context manager. But this is now updated.



""" Private dict of registered IO modules """
PYSTAC_IO: Dict[str, IoReadWrite] = {}

Choose a reason for hiding this comment

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

This is a lot like what python-typeclasses does to track typeclass instances.

pystac_io/__init__.py Outdated Show resolved Hide resolved
scripts/test Outdated Show resolved Hide resolved
Co-authored-by: James Santucci <james.santucci@gmail.com>
@CloudNiner CloudNiner self-assigned this Oct 29, 2020
@CloudNiner
Copy link
Contributor Author

@jisantuc would you be willing to take another look at this? I'd like to merge and get this published to pypi in the next day or so!

Copy link

@jisantuc jisantuc left a comment

Choose a reason for hiding this comment

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

really helpful to have linting + test linting + docs of context manager + tests of context manager available 😎

@CloudNiner CloudNiner merged commit 5b9ff7f into master Oct 30, 2020
@CloudNiner CloudNiner deleted the feature/awf/init branch October 30, 2020 20: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.

None yet

2 participants