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 kwargs keyword only #2661

Merged
merged 38 commits into from
Feb 16, 2023
Merged

Conversation

nkphysics
Copy link
Member

@nkphysics nkphysics commented Feb 9, 2023

This addresses #1746.

At the present time of drafting this PR, refactors to make optional kwargs keyword only have been applied to the following:

  • astroquery/alfalfa
  • astroquery/astrometry_net
  • astroquery/besancon
  • astroquery/casda
  • astroquery/cds
  • astroquery/dace
  • astroquery/exoplanet_orbit_database
  • astroquery/fermi
  • astroquery/gaia
  • astroquery/gama
  • astroquery/gemini
  • astroquery/hips2fits
  • astroquery/hitran
  • astroquery/image_cutouts/first
  • astroquery/imcce (also added # doctest: +IGNORE_OUTPUT for re-emerging issue from BUG: failing remote tests Q1 2023 #2634 that was addressed in MAINT: Fixing HEASARC and IMCCE docs issue #2652 )
  • astroquery/ipac/irsa/ibe
  • astroquery/ipac/irsa/irsa_dust
  • astroquery/ipac/irsa/sha
  • astroquery/ipac/ned
  • astroquery/jplsbdb
  • astroquery/jplspec
  • astroquery/lamda
  • astroquery/linelists/cdms
  • astroquery/magpis
  • astroquery/mpc
  • astroquery/nasa_ads
  • astroquery/oac
  • astroquery/ogle
  • astroquery/open_exoplanet_catalogue
  • astroquery/splatalogue
  • astroquery/template_module
  • astroquery/ukidss
  • astroquery/vsa
  • astroquery/wfau (FYI: has no tests)
  • astroquery/xmatch

Problematic refactors (handle later/ require more scrutiny):

  • astroquery/heasarc (has a few failing tests)
  • astroquery/utls (breaks other packages to the point where all tests should run to make sure everything is working)
  • astroquery/eso (test_each_instrument_SgrAstar is failing)
  • astroquery/cosmosim (has no tests)
  • astroquery/cadc
  • astroquery/utils/tap
  • astroquery/vamdc
  • astroquery/vo_conesearch

@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Merging #2661 (0c9e965) into main (0a67245) will not change coverage.
The diff coverage is 99.00%.

@@           Coverage Diff           @@
##             main    #2661   +/-   ##
=======================================
  Coverage   69.17%   69.17%           
=======================================
  Files         304      304           
  Lines       22524    22524           
=======================================
  Hits        15580    15580           
  Misses       6944     6944           
Impacted Files Coverage Δ
astroquery/ipac/irsa/ibe/tests/test_ibe_remote.py 60.00% <ø> (ø)
astroquery/xmatch/core.py 74.35% <75.00%> (ø)
astroquery/cds/core.py 68.75% <83.33%> (ø)
astroquery/alfalfa/core.py 95.31% <100.00%> (ø)
astroquery/astrometry_net/core.py 47.36% <100.00%> (ø)
astroquery/besancon/core.py 69.43% <100.00%> (ø)
astroquery/casda/core.py 87.80% <100.00%> (ø)
astroquery/dace/core.py 82.92% <100.00%> (ø)
astroquery/eso/core.py 29.09% <100.00%> (ø)
...oplanet_orbit_database/exoplanet_orbit_database.py 93.61% <100.00%> (ø)
... and 37 more

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

@nkphysics
Copy link
Member Author

Would you'll prefer one large PR for this refactoring, or grouping up the refactors into 2-3 smaller PRs to make reviewing a bit easier?

@bsipocz
Copy link
Member

bsipocz commented Feb 10, 2023

One big PR is great, thanks!

@bsipocz bsipocz added this to the v0.4.7 milestone Feb 10, 2023
@nkphysics
Copy link
Member Author

nkphysics commented Feb 13, 2023

CI-devtest failures seem to not be associated with these refactors (https://github.com/astropy/astroquery/actions/runs/4160273194) so I'm just going to proceed with the refactors.

It seems to be a bigger issue with rust compilation in deps.
** New version of pyo3 released last week and maturin yesterday

@bsipocz
Copy link
Member

bsipocz commented Feb 14, 2023

Problematic refactors (need help/feedback, or handle later)

Perfectly fine to leave anything non-trivial for later.

@nkphysics
Copy link
Member Author

Problematic refactors (need help/feedback, or handle later)

Perfectly fine to leave anything non-trivial for later.

If you want I'll resolve the conflicts with gaia and then mark this ready for review, and just tackle the problematic refactors on an individual basis.

@bsipocz
Copy link
Member

bsipocz commented Feb 14, 2023

Sounds good. I've also looked into the diff, and if there is any way to revert and squash all the changes that black did to formatting, it would be awesome.

There are quite a lot of places where it expanded 3 lines into 20 without adding anything to it, and that's the cause of bloating up the diff to the order of hundreds of lines.

As I see most of the commits are fine, and you did it very modularly, so it's also fine to cherry pick the affected ones ou from this branch, etc.

@nkphysics
Copy link
Member Author

Sounds good. I've also looked into the diff, and if there is any way to revert and squash all the changes that black did to formatting, it would be awesome.

There are quite a lot of places where it expanded 3 lines into 20 without adding anything to it, and that's the cause of bloating up the diff to the order of hundreds of lines.

As I see most of the commits are fine, and you did it very modularly, so it's also fine to cherry pick the affected ones ou from this branch, etc.

I'll work on cleaning it up.

@nkphysics nkphysics force-pushed the kwargs-keyword-only branch 2 times, most recently from 10b93fe to 13e2d41 Compare February 15, 2023 00:22
@nkphysics nkphysics marked this pull request as ready for review February 15, 2023 00:30
@nkphysics
Copy link
Member Author

Cleaned this up as best as I could.

@bsipocz
Copy link
Member

bsipocz commented Feb 15, 2023

Cleaned this up as best as I could.

Thanks! The ibe module is the only one left out (4 files), and it will need a rebase given the 2 unrelated commit from main got merged in. However, I'm happy to do the rebase once this is ready to go, once the remote test run finishes (I want to make sure there are no new failures introduced.)

Also, these changes will need a changelog, but I'm thinking about having one changelog that covers all the modules, so we'll just do that before the release listing all the PRs going these kind of changes.

@nkphysics
Copy link
Member Author

Cleaned this up as best as I could.

Thanks! The ibe module is the only one left out (4 files), and it will need a rebase given the 2 unrelated commit from main got merged in. However, I'm happy to do the rebase once this is ready to go, once the remote test run finishes (I want to make sure there are no new failures introduced.)

Also, these changes will need a changelog, but I'm thinking about having one changelog that covers all the modules, so we'll just do that before the release listing all the PRs going these kind of changes.

Ibe is cleaned up, I'll leave the changelog and rebasing up to you.

@bsipocz bsipocz mentioned this pull request Feb 16, 2023
62 tasks
@bsipocz
Copy link
Member

bsipocz commented Feb 16, 2023

Thanks @nkphysics!

@bsipocz bsipocz merged commit c293417 into astropy:main Feb 16, 2023
@nkphysics nkphysics deleted the kwargs-keyword-only branch February 16, 2023 02:15
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.

2 participants