Hello @emellega! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-10-09 10:42:10 UTC

cc @imbasimba

Before diving into any reviews I would point out that there are two test failures that are likely related as I don't see them on main (though I haven't looked into the details).

They are "remote" tests, so CI doesn't pick them up, run them with pytest -P esasky -R

For me they are failing both on this branch and on main. Both failing tests are for the IUE mission, and they are the only tests for that mission. The first test that fails are failing because the request is timing out when trying to fetch the fits file http://sdc.cab.inta-csic.es/ines/jsp/SingleDownload.jsp?filename=LWR13178RL.FITS. I was sort of hoping that this was some temporary problem with the server hosting the IUE data, but since it has been failing for many days now, maybe it's worth investigating some more.

I have contacted the IUE support team regarding the timeouts, but these issues seem unrelated to this particular PR.

There are still some doctest failures in docs/esasky/esasky.rst that are due to changes here. Nothing major, just the change of some methods are now returning dictionaries.

Were can I see those failures? I see a lot of warnings here, but nothing related to my changes from what I can see. I guess it's the list_maps, list_catalogs etc. functions that are problematic? But they don't have the return value specified in the doc string, so why do they generate failures? 🤔

The RTD build would only show rendering related issues that sphinx picks up, but the doctests that validate code example outputs are run as part of the normal test runs.

So for esasky, I would always do a pytest -P esasky -R, that picks up esasky tests from astroquery/esasky/tests as well as doctests from docs/esasky (that's -P is for, so you don't run unrelated modules, and -R do enable the remote data access. This latter is not run for CI on the PR, thus you don't see any of the failures here.

And you can also run tests just for the one file, e.g. here we know the issue is in the doc, so pytest docs/esasky -R would be sufficient.

I would encourage of looking into the tracebacks first and see if the failures are just the case of updating the docs, or better to address the changes on the code level. Once you only have docs related, you maybe able to use the automated updates provided by doctestplus, e.g. run the following and then carefully commit the changes that are relevant:

pytest docs/esasky --doctest-plus-generate-diff=overwrite -R

(We have a long backlog of writing this all up as part of the development documentation, just so you know, it's on the list and will be addressed hopefully soon)

Codecov Report

Attention: Patch coverage is 17.50000% with 66 lines in your changes missing coverage. Please review.

Project coverage is 67.36%. Comparing base (97b5dd3) to head (9f63fe9).
Report is 124 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3106      +/-   ##
==========================================
- Coverage   67.38%   67.36%   -0.02%     
==========================================
  Files         233      233              
  Lines       18417    18406      -11     
==========================================
- Hits        12411    12400      -11     
  Misses       6006     6006              

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

FYI, the IUE team should have addressed the issue now. I could never reproduce it myself, so I cannot verify it. @bsipocz Let me know if you still are experiencing problems with IUE.

Yes, I had a lot of trouble with upper casing and lower casing everything all the time. The first thing I did was to change esasky to stop upper casing and lower casing, so that Spitzer is always Spitzer, and never spitzer or SPITZER. But that meant that almost every test had to change. And since I was trying to change the internals without changing anything with regards to the api, I decided to revert those changes. Also, maybe someone out there has a script that relies on the Spitzer output being in a folder called SPITZER?