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

Geocat-comp ruff cleanup #584

Merged
merged 3 commits into from Mar 28, 2024
Merged

Geocat-comp ruff cleanup #584

merged 3 commits into from Mar 28, 2024

Conversation

cyschneck
Copy link
Contributor

@cyschneck cyschneck commented Mar 25, 2024

PR Summary

Closes #583

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.
  • If needed, squash and merge PR commits into a single commit to clean up commit history

@cyschneck cyschneck self-assigned this Mar 25, 2024
@cyschneck
Copy link
Contributor Author

Currently:

ruff check --per-file-ignores="__init__.py:F401" .
docs/conf.py:15:8: F401 [*] `os` imported but unused
docs/conf.py:25:1: E402 Module level import not at top of file
docs/conf.py:176:1: E402 Module level import not at top of file
geocat/comp/climatologies.py:1:8: F401 [*] `cf_xarray` imported but unused
geocat/comp/meteorology.py:115:9: F841 Local variable `eqtype` is assigned to but never used
Found 5 errors.

The os is not being used in conf.py, but there is reference to how it could be used if there was a need. Should it be removed?

The other two for conf.py are because the imports happen after the comment block, is it alright to move them above the template comment block?

climatologies.py does need cf_xarray (geocat/comp/climatologies.py:30: in _get_time_coordinate_info time = dset.cf["time"]) but it is called as cf instead of cf_xarray so ruff doesn't see how it is being used.

eqtype in the meteorology.py is a variable that is either set to 0 or 1 and then returned at the end for _xheat_index or not when it is used in the _heat_index, so it is technically never used in the _heat_index function on line 115

@cyschneck cyschneck added the testing Issue related to testing label Mar 26, 2024
@cyschneck cyschneck marked this pull request as ready for review March 26, 2024 18:39
@@ -1,6 +1,5 @@
import typing

import cf_xarray
Copy link
Member

Choose a reason for hiding this comment

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

We do actually need the cf_xarray imports. They aren't "used"/called directly, but provide an accessor to some xarray objects.

Copy link
Contributor

@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.

Looks good to me aside from the cf_xarray bit Anissa already caught!

@anissa111 anissa111 merged commit 9ccf3e0 into NCAR:main Mar 28, 2024
15 checks passed
@cyschneck cyschneck deleted the ruff_cleanup_583 branch March 28, 2024 19:17
anissa111 pushed a commit to anissa111/geocat-comp that referenced this pull request Mar 29, 2024
* ruff clean up of unused imports

* update release-notes.rst

* replace required cf_xarray
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Issue related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Geocat-comp ruff cleanup
3 participants