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

Feature/add unit tests #20

Merged
merged 19 commits into from
Jun 14, 2024
Merged

Feature/add unit tests #20

merged 19 commits into from
Jun 14, 2024

Conversation

sray014
Copy link
Collaborator

@sray014 sray014 commented Jun 10, 2024

This PR makes the following changes to ras-stac:

  • Adds unit tests for stac item creation functions
  • Moves reading of s3 hdf files outside of item creation functions
  • Combines ras_hdf and ras_stac into one ras_utils file
  • Organizes item creation functions into 2 classes, one for geom files and one for plan
  • Converts all attribute times into iso format instead of datetime.datetime, removing the need for datetime json encoder

@sray014 sray014 requested a review from mxkpp June 10, 2024 20:08
Copy link
Collaborator

@mxkpp mxkpp left a comment

Choose a reason for hiding this comment

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

In addition to the per-file comments and per-line comments included elsewhere in this review...

Additional comments required to be resolved before merge:

  • Docker setup: Dockerfile fails to build since ras_stac/plugins does not exist.
  • Docker setup: Add a .dockerignore file and use it to explicitly avoid including .env into the image. Even though this is currently not an issue (Dockerfile does not have a COPY . . directive), might as well do it.

Additional comments that could be made into issues if not resolved prior to merge:

  • DRYify Logging: Start a new file ras_stac/utils/logger.py with a function set_up_logger that each module and script can import, then consolidate the various logging calls.
  • Idiomacy: When a variable holds an S3 path / URL having the form "s3://bucket/some/great/blob" Use var name s3_path rather than s3_key or s3_full_key
  • Explicit failures when environment is not set up properly: Use os.environ["VARNAME"] instead of os.environ.get("VARNAME") since the former will fail with a clear KeyError if the env var has not been set.
  • Stability: Consider upgading version of GDAL base image in Dockerfile (currently 3.8.0) to incorporate the latest bug fix releases within 3.8.x.
  • Robustness: Near the top of populate-sample-data.sh, modify existing line set -e to be the standard full set -euo pipefail. Also move this line to the very top, just underneath the shebang, before the source .env.
  • Convenience / Ease of Use: In populate-sample-data.sh, invoke the Python runtime like python -um ... instead of python -m .... The u causes Python to run unbuffered, so you can see lines in realtime as they are printed/logged rather than waiting for a large chunk of lines to flush.

ras_stac/utils/dg_utils.py Outdated Show resolved Hide resolved
ras_stac/utils/dg_utils.py Outdated Show resolved Hide resolved
ras_stac/utils/ras_utils.py Outdated Show resolved Hide resolved
ras_stac/utils/s3_utils.py Outdated Show resolved Hide resolved
ras_stac/utils/s3_utils.py Outdated Show resolved Hide resolved
ras_stac/utils/s3_utils.py Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
@sray014 sray014 requested a review from mxkpp June 13, 2024 16:31
Copy link
Collaborator

@mxkpp mxkpp 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 the updates. I just pushed a small change to an error message, good to go.

Edit: I did push one more commit to add core tests into the Docker image, and to add a shell script for building the Docker image and running those core tests in the container.

@sray014 sray014 merged commit 9d78ecc into main Jun 14, 2024
4 checks passed
@sray014 sray014 deleted the feature/add_unit_tests branch June 14, 2024 18:53
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