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

Cadc and Heasarc Kwarg refactors #2671

Merged
merged 4 commits into from
Feb 27, 2023

Conversation

nkphysics
Copy link
Member

  • Did refactors to make kwargs keyword only for Heasarc and Cadc.(Use keyword only arguments #1746)
  • Heasarc' test_mission_cols was failing where it was looking for the incorrect column name (SEARCH_OFFSET instead of SEARCH_OFFSET)
  • Heasarc also had doc failures again with docs which were addressed before MAINT: Fixing HEASARC and IMCCE docs issue #2652, so added an ignore_output to the doctest in question which was posing issues. Seems to be an issue on integrals end

@nkphysics
Copy link
Member Author

There seems to be a discrepancy between the way SEARCH_OFFSET is being returned. On my machine test_mission_cols fails since the column returned is SEARCH_OFFSET_, rather than _SEARCH_OFFSET which is how it is being returned on the ci runner.

@nkphysics
Copy link
Member Author

  • Heasarc' test_mission_cols was failing where it was looking for the incorrect column name (SEARCH_OFFSET instead of SEARCH_OFFSET)

Removed this and tests passed here. They're still failing locally for me for some reason. Could be the python version, maybe something might have been cached from some of my other work that uses astroquery as a dep (although unlikely cause I run everything in its own venv).
I'm only putting this comment in here cause idk if someone else encountered this.

@nkphysics nkphysics marked this pull request as ready for review February 17, 2023 17:58
@codecov
Copy link

codecov bot commented Feb 17, 2023

Codecov Report

Merging #2671 (514cd66) into main (e388ceb) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 514cd66 differs from pull request most recent head 6eb5dc4. Consider uploading reports for the commit 6eb5dc4 to get more accurate results

@@           Coverage Diff           @@
##             main    #2671   +/-   ##
=======================================
  Coverage   69.17%   69.17%           
=======================================
  Files         304      304           
  Lines       22524    22524           
=======================================
  Hits        15580    15580           
  Misses       6944     6944           
Impacted Files Coverage Δ
astroquery/cadc/core.py 81.11% <100.00%> (ø)
astroquery/heasarc/core.py 75.48% <100.00%> (ø)

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

@nkphysics nkphysics changed the title Cadc and Heasarc Kwarg refactors and Heasarc test fixes Cadc and Heasarc Kwarg refactors Feb 22, 2023
@bsipocz bsipocz added this to the v0.4.7 milestone Feb 23, 2023
@bsipocz
Copy link
Member

bsipocz commented Feb 23, 2023

cc @andamian - to confirm you're OK with this for cadc

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.

Looks good.

I'll rebase though to get rid of the merge commit, after we got the cadc approval, too.

@bsipocz
Copy link
Member

bsipocz commented Feb 23, 2023

@nkphysics - The usual way to keep a branch up-to-date compared to the default branch is done with a rebase, rather than a merge/pull, which comes easier if you branch out from "upstream"'s main rather than your fork's main. (In fact, I don't even keep my forks main synced up, in fact even got rid of it).

@andamian
Copy link

Looks good to me. The only thing I suggest is to set the milestone to 0.5 as this change in the API is not backwards compatible. Users should be instructed what to do when they encounter TypeError. @nkphysics - thanks for the contribution.

@andamian andamian self-requested a review February 23, 2023 20:31
@bsipocz
Copy link
Member

bsipocz commented Feb 23, 2023

The only thing I suggest is to set the milestone to 0.5 as this change in the API is not backwards compatible.

Version numbers here mean nothing really, but I hear your suggestion that the next one should be 0.5 :)
(or maybe, for the next one we get to manage to figure out a new numbering system, I'm more and more inclined towards something date based as we cannot do anything semver due to the inability to offer backward combatibility)

@nkphysics
Copy link
Member Author

@nkphysics - The usual way to keep a branch up-to-date compared to the default branch is done with a rebase, rather than a merge/pull, which comes easier if you branch out from "upstream"'s main rather than your fork's main. (In fact, I don't even keep my forks main synced up, in fact even got rid of it).

Gotcha, thanks for the guidance.

@ceb8 ceb8 merged commit 5e221bc into astropy:main Feb 27, 2023
@ceb8
Copy link
Member

ceb8 commented Feb 27, 2023

Thanks @nkphysics!

@nkphysics nkphysics deleted the kwarg-refactors-test-fixes branch February 28, 2023 00:20
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.

None yet

4 participants