search

astropy / extension-helpers

Helpers to assist with building Python packages with compiled C/Cython extensions
https://extension-helpers.readthedocs.io
BSD 3-Clause "New" or "Revised" License
16 stars 12 forks source link

Support pathlib.Path in write_if_different and import_file #84

Closed astrofrog closed 2 months ago

astrofrog commented 2 months ago

Replacement for https://github.com/astropy/extension-helpers/pull/83/ which includes tests

codecov[bot] commented 2 months ago

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 15 commits behind head on main.

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

neutrinoceros commented 2 months ago

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

astrofrog commented 2 months ago

@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 commented 2 months ago

The tests here also check both str and Path work, so I'd prefer to keep https://github.com/astropy/extension-helpers/pull/84 and close https://github.com/astropy/extension-helpers/pull/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 commented 2 months ago

I didn't drop support for str in my PR

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

astrofrog commented 2 months ago

@neutrinoceros - are you happy with this now?