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

Support pathlib.Path in write_if_different and import_file #84

Merged
merged 3 commits into from
Sep 5, 2024

Conversation

astrofrog
Copy link
Member

Replacement for #83 which includes tests

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 75.98%. Comparing base (fbd3ba0) to head (c5c51ff).
Report is 25 commits behind head on main.

Files with missing lines Patch % Lines
extension_helpers/_utils.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #84   +/-   ##
=======================================
  Coverage   75.98%   75.98%           
=======================================
  Files           4        4           
  Lines         329      329           
=======================================
  Hits          250      250           
  Misses         79       79           

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

@neutrinoceros
Copy link
Contributor

@astrofrog Looks like we both worked on this at the very same time (#85) !
My PR looks simpler though, what do you think ?

@astrofrog
Copy link
Member Author

astrofrog commented Sep 5, 2024

@neutrinoceros - we need to keep supporting str input for those functions, hence some of the changes I made here, and I've also been able to simplify a little of the code using read_bytes/write_bytes. I've also updated the docstrings. The tests here also check both str and Path work, so I'd prefer to keep #84 and close #85 if you agree.

@neutrinoceros
Copy link
Contributor

The tests here also check both str and Path work, so I'd prefer to keep #84 and close #85 if you agree.

I didn't drop support for str in my PR, and I also have tests for both path flavors, so both PRs are pretty much equivalent there, but yours does have additional documentation change so it should probably be favored, yes.
My one critic here is that you're changing the name of an argument that's keyword-allowed, which seems like an unwarranted breaking change.

@astrofrog
Copy link
Member Author

I didn't drop support for str in my PR

Sorry I wrote my comment before you pushed some additional changes 😅 - race conditions!

@@ -77,7 +78,7 @@ def walk_skip_hidden(top, onerror=None, followlinks=False):
yield root, dirs, files


def write_if_different(filename, data):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the breaking change I was talking about. Can you please revert it ? Beyond that, happy to approve this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done and pushed

@astrofrog
Copy link
Member Author

@neutrinoceros - are you happy with this now?

Copy link
Contributor

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Absolutely. Thank you !

@astrofrog astrofrog merged commit 6fa2009 into astropy:main Sep 5, 2024
19 checks passed
@astrofrog astrofrog added the enhancement New feature or request label Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants