Skip to content

Make astroquery.mast methods take explicit kwargs #2317

Merged
bsipocz merged 3 commits into
astropy:mainfrom
jaymedina:explicit-kwargs
Mar 8, 2022
Merged

Make astroquery.mast methods take explicit kwargs #2317
bsipocz merged 3 commits into
astropy:mainfrom
jaymedina:explicit-kwargs

Conversation

@jaymedina

@jaymedina jaymedina commented Mar 2, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Comment thread astroquery/mast/observations.py Outdated
@bsipocz

bsipocz commented Mar 2, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

Would this need a changelog entry? @bsipocz

@bsipocz

bsipocz commented Mar 2, 2022

Copy link
Copy Markdown
Member

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
Copy Markdown
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

codecov Bot commented Mar 7, 2022

Copy link
Copy Markdown

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

bsipocz commented Mar 8, 2022

Copy link
Copy Markdown
Member

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