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

Adding tests to Util #239

Merged
merged 10 commits into from
Jul 9, 2024
Merged

Adding tests to Util #239

merged 10 commits into from
Jul 9, 2024

Conversation

jukent
Copy link
Collaborator

@jukent jukent commented Jun 25, 2024

PR Summary

First pass at tests for util.py
Closes #126

PR Checklist

General

  • Make an issue if one doesn't already exist
  • Link the issue this PR resolves by adding closes #XXX to the PR description where XXX is the number of the issue.
  • Add a brief summary of changes to docs/release-notes.rst in a relevant section for the next unreleased release. Possible sections include: Documentation, New Features, Bug Fixes, Internal Changes, Breaking Changes/Deprecated
  • Add appropriate labels to this PR
  • Make your changes in a forked repository rather than directly in this repo
  • Open this PR as a draft if it is not ready for review
  • Convert this PR from a draft to a full PR before requesting reviewers
  • Passes precommit. To set up on your local, run pre-commit install from the top level of the repository. To manually run pre-commits, use pre-commit run --all-files and re-add any changed files before committing again and pushing.

@jukent jukent added the work in progress Not ready for review yet label Jun 25, 2024
@jukent
Copy link
Collaborator Author

jukent commented Jun 25, 2024

Currently there are some place holder functions in here and I did not generate the requisite images yet, just creating this PR so that my work so far is visible.

@jukent
Copy link
Collaborator Author

jukent commented Jun 27, 2024

I would love to replace some of the datafiles accessed will simpler dummy data, but because of the lat/lon grids and skewt plots, it seemed easier to access these small datafiles.

@jukent jukent marked this pull request as ready for review June 27, 2024 23:11
@jukent jukent requested a review from kafitzgerald June 27, 2024 23:11
@jukent jukent added developer feature For development standardization / best practices / enhancement and removed work in progress Not ready for review yet labels Jun 27, 2024
docs/release-notes.rst Outdated Show resolved Hide resolved
Co-authored-by: Katelyn FitzGerald <7872563+kafitzgerald@users.noreply.github.com>
Copy link
Collaborator

@kafitzgerald kafitzgerald left a comment

Choose a reason for hiding this comment

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

Sorry for the individual suggestions before - I meant to include those in the review.

This looks like a great start to me! Obviously, things at could be improved / added, but I think just getting this merged ASAP probably makes sense.

I think my main question/concern is about the imports (see the inline comment).

And then a minor thing - should the tests/test.ipynb be removed? It looks like an empty file that got added by accident.


from geocat.viz.util import truncate_colormap
import geocat.viz.util as gv
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was there a reason for switching this up? The convention I've typically seen for code being tested is the explicit "from x import y" for all tested functions and classes. Generally at the bottom of the imports section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not realize that, I thought importing every single util function would take up a lot of space, so I imported the module instead. I see how that would make it more clear right away.

@jukent jukent requested a review from kafitzgerald July 9, 2024 17:07
Copy link
Collaborator

@kafitzgerald kafitzgerald left a comment

Choose a reason for hiding this comment

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

🥳

@jukent jukent merged commit bc16dde into NCAR:main Jul 9, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer feature For development standardization / best practices / enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible Testing Schemes
2 participants