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

Fix chdir and add tests for fsutils functions #45

Merged
merged 8 commits into from
Nov 15, 2024

Conversation

DavidHuber-NOAA
Copy link
Collaborator

Description

This modifies chdir so that it produces an error if the target directory is not available and returns to the original working directory when the with completes without producing a warning.

Tests were added to verify that chdir, as well as the other fsutils, run as expected. While writing these tests, the following issues were found and fixed:

  • mkdir_p was simplified to use the os.mkdir exist_ok=True parameter instead of attempting to handle that in wxflow.
  • Added a missing_ok parameter to rmdir to allow missing directories to be skipped without error.
  • Improved rm_p error handling and added a missing_okparameter.

Lastly, the only function that did not have a test added was chgrp. I am not sure what groups are available on GitHub runners, so I am not sure how this could be tested reliably.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New tests

How Has This Been Tested?

  • pynorms
  • pytests

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing tests pass with my changes

if missing_ok:
logger.warning(f"WARNING cannot remove the target path {dir_path} because it does not exist")
else:
raise exc
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise exc
raise FileNotFoundError()

Should this raise this error specifically. Not sure how this works, if its trying to remove something that is not existant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since exc is the original FileNotFoundError, raising exc is equivalent to raising FileNotFoundError. But, raising a custom FileNotFoundError with an more specific error message would be useful. I'll add that in.

So if the file doesn't exist, nothing will happen but rmtree will return a FileNotFoundError. If missing_ok=False, then this Exception will be passed on. Otherwise, you get a warning for trying.

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 52.41%. Comparing base (e1ef697) to head (6524b8b).
Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
src/wxflow/fsutils.py 83.33% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #45      +/-   ##
===========================================
+ Coverage    50.12%   52.41%   +2.29%     
===========================================
  Files           18       18              
  Lines         1652     1679      +27     
  Branches       339      296      -43     
===========================================
+ Hits           828      880      +52     
+ Misses         765      749      -16     
+ Partials        59       50       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

lgtm. thanks for beefing up fsutils and adding tests.

@DavidHuber-NOAA DavidHuber-NOAA merged commit a7b49e9 into NOAA-EMC:develop Nov 15, 2024
9 checks passed
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.

2 participants