Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactors: Make kwargs keyword only for remaining packages #2703

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

nkphysics
Copy link
Member

@nkphysics nkphysics commented Apr 10, 2023

This PR is to address the remaining packages requiring refactors to make kwargs keyword only (#1746).

From the list in #1746

Hoping for some feedback on vamdc and cosmosim as I'm a little confused as to what's going on there or what you'll think is the best way to proceed.

@codecov
Copy link

codecov bot commented Apr 10, 2023

Codecov Report

Merging #2703 (af27a78) into main (4d0bb53) will not change coverage.
The diff coverage is 92.85%.

@@           Coverage Diff           @@
##             main    #2703   +/-   ##
=======================================
  Coverage   65.73%   65.73%           
=======================================
  Files         233      233           
  Lines       17842    17842           
=======================================
  Hits        11729    11729           
  Misses       6113     6113           
Impacted Files Coverage Δ
astroquery/vo_conesearch/validator/validate.py 19.69% <50.00%> (ø)
astroquery/vo_conesearch/validator/inspect.py 100.00% <100.00%> (ø)
astroquery/vo_conesearch/validator/tstquery.py 32.00% <100.00%> (ø)
astroquery/vo_conesearch/vo_async.py 38.88% <100.00%> (ø)
astroquery/vo_conesearch/vos_catalog.py 69.51% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nkphysics nkphysics marked this pull request as ready for review April 27, 2023 19:12
@nkphysics
Copy link
Member Author

I'm marking this ready for review.
I can address astroquery/vamdc and astroquery/cosmosim once I know if we want refactors for them despite there being no tests for cosmosim, and the deprecation status of vamdc.

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. None of these changes look like they would break any standard use cases.

@bsipocz
Copy link
Member

bsipocz commented Apr 28, 2023

I can address astroquery/vamdc and astroquery/cosmosim once I know if we want refactors for them despite there being no tests for cosmosim, and the deprecation status of vamdc.

It's more than OK to skip vamdc (@keflavich - we really need to do something about it), but if you don't mind to add cosmosim, it would be great, so the we can say all is done.

Edit: ahh, I see your more detailed comment above in the OP about cosmosim. Sure, it's OK to skip it for now, and I would say that module needs an audit and either adding tests (at this point I'm not sure whether to resurrect either of the tests PRs or writing new ones is easier, but it does need some realistic test suite. Enabling testing on the docs could be an option, too). If testing is not possible, we could even consider deprecating it.

@bsipocz bsipocz added this to the v0.4.7 milestone Apr 28, 2023
@bsipocz bsipocz merged commit 630686c into astropy:main Apr 28, 2023
8 checks passed
@bsipocz
Copy link
Member

bsipocz commented Apr 28, 2023

Thanks @nkphysics!

@nkphysics nkphysics deleted the kwarg-konly branch April 28, 2023 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants