radio-astro-tools / spectral-cube

Library for reading and analyzing astrophysical spectral data cubes
http://spectral-cube.rtfd.org
BSD 3-Clause "New" or "Revised" License
95 stars 62 forks source link

Deprecation fix the 2nd: replace `mktemp` with `TemporaryDirectory` #796

Open dhomeier opened 2 years ago

dhomeier commented 2 years ago

Another go at replacing the deprecated function for the save_to_tmp_dir option, with an attempt to delete the temporary files along with the cube instance. See however discussion about using __del__ in https://github.com/radio-astro-tools/spectral-cube/pull/789#pullrequestreview-847262861.

codecov-commenter commented 2 years ago

Codecov Report

Merging #796 (4b72221) into master (c7281a7) will decrease coverage by 0.00%. The diff coverage is 37.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #796      +/-   ##
==========================================
- Coverage   78.00%   78.00%   -0.01%     
==========================================
  Files          24       24              
  Lines        5847     5850       +3     
==========================================
+ Hits         4561     4563       +2     
- Misses       1286     1287       +1     
Impacted Files Coverage Δ
spectral_cube/dask_spectral_cube.py 85.54% <37.50%> (-0.10%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c7281a7...4b72221. Read the comment docs.

dhomeier commented 2 years ago

Apparently the Windows test is still the only one that runs with zarr installed and not skipping the tests. Don't quite understand why, as it's also listed in the novis deps.

dhomeier commented 2 years ago

Similarly, same process, but close python w/o deleting tempdir.

Good point, something I've also only tested locally under macOS. Not sure if this can be implemented in the CI – I think open_files is kind of testing this, but may not be completely equivalent. And I have no idea about tmpfile handling on Windows in general.

keflavich commented 1 year ago

@dhomeier could you rebase this? I think we're ready to merge even though I never did end up doing that manual test