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

Refactor: make Vizier kwargs keyword-only #2610

Merged
merged 4 commits into from
Nov 27, 2022

Conversation

keflavich
Copy link
Contributor

See #1746.

This refactor raises a question: should we keyword-only the "private" functions too?

@codecov
Copy link

codecov bot commented Nov 27, 2022

Codecov Report

Merging #2610 (283c8c4) into main (2ac0cb8) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 283c8c4 differs from pull request most recent head c024d54. Consider uploading reports for the commit c024d54 to get more accurate results

@@           Coverage Diff           @@
##             main    #2610   +/-   ##
=======================================
  Coverage   64.20%   64.20%           
=======================================
  Files         130      130           
  Lines       16892    16892           
=======================================
  Hits        10845    10845           
  Misses       6047     6047           
Impacted Files Coverage Δ
astroquery/vizier/core.py 78.59% <100.00%> (ø)

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

@bsipocz
Copy link
Member

bsipocz commented Nov 27, 2022

Hahh, was about to push the same changes 😄

I go ahead and merge this, so the new PR can be on top of this.

@bsipocz bsipocz added this to the v0.4.7 milestone Nov 27, 2022
@bsipocz bsipocz added the vizier label Nov 27, 2022
@keflavich
Copy link
Contributor Author

There's a bug in the docs that I was working on - should I put this down for now?

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

Minor fix for sphinx

astroquery/vizier/core.py Outdated Show resolved Hide resolved
@bsipocz
Copy link
Member

bsipocz commented Nov 27, 2022

There's a bug in the docs that I was working on - should I put this down for now?

I didn't touch the docs, we don't yet have it included in the tests, so it doesn't come up as a failure.

@keflavich
Copy link
Contributor Author

I meant the docstr fix you suggested - I pushed it before I saw your suggestion =)

@keflavich
Copy link
Contributor Author

What about changelog? Should we individually changelog these?

@bsipocz
Copy link
Member

bsipocz commented Nov 27, 2022

Yes, changelog is needed, but I just go ahead with the merge and include its changelog in the new PR.

@bsipocz bsipocz merged commit 0cc58b7 into astropy:main Nov 27, 2022
@bsipocz bsipocz mentioned this pull request Nov 27, 2022
62 tasks
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

2 participants