Hello @jaymedina! 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 2022-01-26 17:23:50 UTC

@jaymedina - I've converted this to a draft PR, please change it back once it's out of the WIP state and ready for review.

Codecov Report

Merging #2193 (0f93106) into main (c933c3d) will increase coverage by 0.01%.
The diff coverage is 93.10%.

❗ Current head 0f93106 differs from pull request most recent head 45d7ab8. Consider uploading reports for the commit 45d7ab8 to get more accurate results

@@            Coverage Diff             @@
##             main    #2193      +/-   ##
==========================================
+ Coverage   62.69%   62.70%   +0.01%     
==========================================
  Files         131      131              
  Lines       16822    16828       +6     
==========================================
+ Hits        10546    10552       +6     
  Misses       6276     6276              

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 c933c3d...45d7ab8. Read the comment docs.

Is there any thought to having this this accept environment variables as configuration overrides? If we allowed for environment variables it would make it much easier to reconfigure without the need to make code changes to people's scripts and potentially easier to test, as well.

Is there any thought to having this this accept environment variables as configuration overrides? If we allowed for environment variables it would make it much easier to reconfigure without the need to make code changes to people's scripts and potentially easier to test, as well.

Some of these attributes are "built" off attributes that are inherited from other imports, so I'm not sure if that would make things cleaner. For example MAST_REQUEST_URL = conf.server + "/api/v0/invoke" under PortalAPI, as an example, uses conf.server taken from the conf import (config file). I also worry people might overwrite the environment variables by mistake, or there might be a typo somewhere that breaks the scripts. But maybe Tom or Brigitta have another take.

Will need to patch the services.py URLs as well

I've addressed all previous threads and installed the final "hooks" that the STScI devs need for unit testing patchwork. I've been running my unit tests successfully off this branch, so I'm going to mark this as Ready for Review. There's some new code that's been modified under services.py so I'd appreciate it if that is looked over to make sure there are no typos/skips in logic.

Thanks for taking some time out to review this already @bsipocz @tomdonaldson @dr-rodriguez and @falkben , feel free to send back any further comments.

With the OK from you @bsipocz I'd like to do a rebase and rerun the CI. I already created a backup branch just in case.

@jaymedina - please go ahead. I'll do a quick review afterwards.

The latest readthedocs build is failing with an error from another module called esa:

astroquery/docs/api/astroquery.esa.jwst.JwstClass.rst:23: WARNING: autosummary: failed to import JwstClass.login_gui
WARNING: autodoc: failed to import method 'JwstClass.login_gui' from module 'astroquery.esa.jwst'; the following exception was raised:

[...]

AttributeError: type object 'JwstClass' has no attribute 'login_gui'

I'm looking at the most recently merged PR and think it might be the source of the bug: #2260

I don't see any CI tests, just the code coverage report.

Side note: astroquery.mast unit tests are passing on this branch

test_mast.py .........................................                   [100%]

============================== 41 passed in 0.93s ==============================

test_mast_remote.py ..................s....................              [100%]

================== 38 passed, 1 skipped in 238.54s (0:03:58) ===================

Hmm, thanks for catching this. That PR you refer has a fully passing CI prior the merge, I'm not sure what went wrong. Nevertheless, it's indeed unrelated to this one.

The latest readthedocs build is failing with an error from another module called esa:

astroquery/docs/api/astroquery.esa.jwst.JwstClass.rst:23: WARNING: autosummary: failed to import JwstClass.login_gui
WARNING: autodoc: failed to import method 'JwstClass.login_gui' from module 'astroquery.esa.jwst'; the following exception was raised:

[...]

AttributeError: type object 'JwstClass' has no attribute 'login_gui'

There is no such error anywhere in the documentation build log. See #2262 for the successful fix.

I've received that error when building the docs locally in this branch, and a freshly checked out branch from main. Regarding #2262, how is that PR related to this?

Some of the cached files in your docs/api/ or docs/_build/ have become outdated. Delete both directories to force Sphinx to rebuild them from scratch.

Regarding #2262, how is that PR related to this?

It fixes the documentation build.

Hmm unfortunately that didn't work @eerovaher . I removed api and _build . Does it work on your end?

Edit: Now receiving this warning:

WARNING: Astrometry.net API key not found in configuration file [astroquery.astrometry_net.core]
WARNING: You need to manually edit the configuration file and add it [astroquery.astrometry_net.core]
WARNING: You may also register it for this session with AstrometryNet.key = 'XXXXXXXX' [astroquery.astrometry_net.core]
/home/docs/checkouts/readthedocs.org/user_builds/astroquery/envs/2193/lib/python3.8/site-packages/astroquery/fermi/__init__.py:37: UserWarning: Experimental: Fermi-LAT has not yet been refactored to have its API match the rest of astroquery.

Deleting the docs/api/ and docs/_build/ directories was meant to ensure that the error regarding JwstClass not having attribute login_gui stops being an issue. It wasn't supposed address the other warnings.

(unrelated to the PR, but I think good rule of thumbs: If you run into weird issues with docs build/tests, running a git clean is a useful command to clean up everything not in the repo (be careful, and exclude any draft or notes or ide directory that you may have, e.g. this is the one I usually run git clean -dfx -e .idea -e notes -e benchmarks. Alternatively usually make clean in the docs directory also works).

Just rebased on the new merged changes and readthedocs build is working again, thanks @eerovaher & @bsipocz ! CI checks are passing and unit tests are passing (remote included). I think this PR is ready for a final review and merge. Thanks in advance.

Thanks @jaymedina!