Refactor reference file loading

[hpparvi]

This PR addresses Issue #169 (also discussed in PR #165) by using the astropy.config.ConfigNamespace.set_temp context manager to temporarily set the default data download URL and then using either astropy.utils.data.get_pkg_data_fileobj or astropy.utils.data.get_pkg_data_filename to get a readable file or a path to the downloaded and cached file.

However, I'm not convinced this approach leads to more elegant code than the original one. We eliminate the two wrapper functions, but the file-handling code is somewhat messy because of all the nesting. Please take a look and let me know what you think.

[codecov]

Codecov Report

Attention: Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.77%. Comparing base (ef38dc0) to head (9aecda5).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
specreduce/calibration_data.py	92.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
+ Coverage   83.36%   84.77%   +1.41%     
==========================================
  Files          13       13              
  Lines        1136     1123      -13     
==========================================
+ Hits          947      952       +5     
+ Misses        189      171      -18     

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

[hpparvi]

Do you have any opinions about this, @cshanahan1, @tepickering, and @kecnry? If nobody opposes this approach, I'll improve the code coverage and see if I can make the code a bit more readable.

[tepickering]

thanks for catching this! i filed the original issue, but had long since forgotten about it 😅. i like what you've done so far. will give it a closer look when you're more ready to merge. a few test failures to address as well as the coverage.

[hpparvi]

Thanks, @tepickering. However, the more I looked at the approach, the less I liked it. The code's readability decreased too much without any improvements in the functionality. I've now switched back to using download_file consistently but without the wrappers (and added a couple of new tests checking that we catch URLErrors from bad file names correctly), which leads to cleaner and more readable code.

The PR is ready to merge in theory, but the code doesn't differ much from the original. It mainly just skips the use of the download wrapper functions. If you still think this is worth merging, doing a squash merge is probably best because most of the intermediate commits were very small.

[tepickering]

looks good. my only comment of note is that some of the diffs are due to changes in code style. it may be worth following what astropy does and use black and/or ruff to define the style and apply it automatically and consistently.

[tepickering]

Thanks, @tepickering. However, the more I looked at the approach, the less I liked it. The code's readability decreased too much without any improvements in the functionality. I've now switched back to using download_file consistently but without the wrappers (and added a couple of new tests checking that we catch URLErrors from bad file names correctly), which leads to cleaner and more readable code.

The PR is ready to merge in theory, but the code doesn't differ much from the original. It mainly just skips the use of the download wrapper functions. If you still think this is worth merging, doing a squash merge is probably best because most of the intermediate commits were very small.

it might, in fact, be worthwhile to make squash merging the default going forward. right now all three methods are allowed and i can't see how to change the default to "squash". i think it becomes the default if i disable "merge commit", but not sure if we want to pull that trigger yet. i'm fine with keeping our options open and deciding on a per-PR basis, but i'm open to other ideas.