Skip to content

Conversation

jaymedina
Copy link
Contributor

@jaymedina jaymedina commented Mar 2, 2022

Make methods for the astroquery.mast services (Observations, Catalogs, Cutouts) take explicit keyword arguments to avoid hiccups in the API calls that we've seen in the past. Closes #2267.

make kwargs explicit:

  • Observations
  • Catalogs
  • Cutouts
  • update tests
  • changelog entry

@jaymedina
Copy link
Contributor Author

Before I continue @bsipocz I wanted to verify with you that my first commit is what you had in mind. The mandatory argument is set to explicit while optional arguments are not. I did this because I figure it would get tedious for users to manually input pageSize and page parameters when they don't actually need it for the call.

@jaymedina jaymedina marked this pull request as draft March 2, 2022 20:48
@bsipocz
Copy link
Member

bsipocz commented Mar 2, 2022

Before I continue @bsipocz I wanted to verify with you that my first commit is what you had in mind

glad you asked, see the inline comment. See also https://github.com/astropy/astroquery/pull/2309/files

@jaymedina
Copy link
Contributor Author

Would this need a changelog entry? @bsipocz

@bsipocz
Copy link
Member

bsipocz commented Mar 2, 2022

Yes, a short one, e.g. Optional keyword arguments are now keyword only. would be useful. I may merge that for the multiple modules before the release.

Also, please squash out the first commit as it's better not to keep those syntax errors in history.

@bsipocz bsipocz added this to the v0.4.6 milestone Mar 2, 2022
@jaymedina
Copy link
Contributor Author

Yes, a short one, e.g. Optional keyword arguments are now keyword only. would be useful. I may merge that for the multiple modules before the release.

Also, please squash out the first commit as it's better not to keep those syntax errors in history.

Sounds good!

@jaymedina jaymedina force-pushed the explicit-kwargs branch 3 times, most recently from a5d1d1c to b0a1637 Compare March 2, 2022 21:57
@codecov
Copy link

codecov bot commented Mar 7, 2022

Codecov Report

Merging #2317 (5de8905) into main (3342888) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2317   +/-   ##
=======================================
  Coverage   62.98%   62.98%           
=======================================
  Files         131      131           
  Lines       17059    17059           
=======================================
  Hits        10745    10745           
  Misses       6314     6314           
Impacted Files Coverage Δ
astroquery/mast/collections.py 90.25% <100.00%> (ø)
astroquery/mast/cutouts.py 79.42% <100.00%> (ø)
astroquery/mast/observations.py 76.41% <100.00%> (ø)

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 3342888...5de8905. Read the comment docs.

@jaymedina jaymedina marked this pull request as ready for review March 7, 2022 22:29
@bsipocz
Copy link
Member

bsipocz commented Mar 8, 2022

Thanks @jaymedina!

(I confirm that all remote and docs tests pass, too. Though they take a long time (35mins), so we may want to think about how to cut back on them, whether that's possible without sacrificing coverage).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make astroquery.mast.cutouts arguments kwargs only
2 participants