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

rename cds into mocserver #2766

Merged
merged 7 commits into from
Jul 7, 2023

Conversation

ManonMarchand
Copy link
Member

This PR is linked to the discussion we had in #2764
Please comment because I was unsure about the best way to inform of the deprecation without breaking API too much.

For now, the behaviour is that when a user does :

>>> from astroquery.cds import cds

There is a warning but the class is still imported with its old name:

<stdin>:1: DeprecationWarning: The ``cds`` module has been moved to astroquery.mocserver,and ``CdsClass`` has been renamed ``MOCserver``. It means that 'from astroquery.cds import cds' became 'from astroquery.mocserver import mocserver'. Please update your imports.
>>> cds.
cds.TIMEOUT                 cds.URL                     cds.cache_location          cds.clear_cache()           cds.find_datasets(          cds.name                    cds.path_moc_file           cds.query_region(           cds.query_region_async(     cds.reset_cache_location()  cds.return_moc

However, if the import is done with import astroquery only, the users code will be broken.
Is there a better way to do this?

Thanks a lot for everything astroquery does <3

PS: the MOCserver module will receive some love from CDS soon because there is now the possibility to query TimeMOC and FrequencyMOC on our side and we'd like to update the library here too

@ManonMarchand ManonMarchand marked this pull request as draft July 6, 2023 12:55
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #2766 (67d2f5f) into main (a99ad27) will increase coverage by 0.00%.
The diff coverage is 76.47%.

❗ Current head 67d2f5f differs from pull request most recent head 4a48005. Consider uploading reports for the commit 4a48005 to get more accurate results

@@           Coverage Diff           @@
##             main    #2766   +/-   ##
=======================================
  Coverage   66.10%   66.11%           
=======================================
  Files         234      235    +1     
  Lines       18057    18060    +3     
=======================================
+ Hits        11937    11940    +3     
  Misses       6120     6120           
Impacted Files Coverage Δ
astroquery/cds/__init__.py 50.00% <40.00%> (-50.00%) ⬇️
astroquery/mocserver/core.py 70.21% <75.00%> (ø)
astroquery/mocserver/__init__.py 100.00% <100.00%> (ø)

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

@ManonMarchand ManonMarchand marked this pull request as ready for review July 6, 2023 13:53
@bsipocz bsipocz added this to the v0.4.7 milestone Jul 6, 2023
@bsipocz
Copy link
Member

bsipocz commented Jul 6, 2023

PS: the MOCserver module will receive some love from CDS soon because there is now the possibility to query TimeMOC and FrequencyMOC on our side and we'd like to update the library here too

❤️

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.

I have some minor comments mostly about naming things, otherwise it all looks good.

If you have some time for a follow-up, definitely not part of this PR: I've noticed that the docs is still skipped for testing, fixing that would be superb. (other modules can be used as an example, basically removing the skip directives and using the doctest-remote-data directive, and in the meantime fixing any non-matching outputs).

Also, I noticed one of the remote tests run for a rather long time, I wonder whether it could cut back to something quicker/smaller? (to run all the tests for this module took me 8.5 minutes)

CHANGES.rst Outdated Show resolved Hide resolved
astroquery/cds/__init__.py Outdated Show resolved Hide resolved
astroquery/cds/__init__.py Outdated Show resolved Hide resolved
astroquery/cds/__init__.py Outdated Show resolved Hide resolved
astroquery/mocserver/core.py Outdated Show resolved Hide resolved
@ManonMarchand ManonMarchand marked this pull request as draft July 7, 2023 08:39
@ManonMarchand
Copy link
Member Author

I have some minor comments mostly about naming things, otherwise it all looks good.

Thanks for the review! I think I addressed your comments. Let me know if I got them wrong.

If you have some time for a follow-up, definitely not part of this PR: I've noticed that the docs is still skipped for testing, fixing that would be superb. (other modules can be used as an example, basically removing the skip directives and using the doctest-remote-data directive, and in the meantime fixing any non-matching outputs).

Also, I noticed one of the remote tests run for a rather long time, I wonder whether it could cut back to something quicker/smaller? (to run all the tests for this module took me 8.5 minutes)

Definitely adding these two things to my list. Won't be before the end of the summer holiday though.

@ManonMarchand ManonMarchand marked this pull request as ready for review July 7, 2023 10:03
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.

A few very minor suggestions that I'll just directly commit to the branch, and then merge when we get the new green CI status.

Thank you very much for the quick turnaround with the PR!

astroquery/cds/__init__.py Outdated Show resolved Hide resolved
astroquery/cds/__init__.py Show resolved Hide resolved
astroquery/cds/__init__.py Show resolved Hide resolved
@bsipocz bsipocz force-pushed the rename-cds-into-mocserver branch from 4a48005 to d9b1e3c Compare July 7, 2023 17:19
@bsipocz bsipocz merged commit e9d229f into astropy:main Jul 7, 2023
7 checks passed
@bsipocz
Copy link
Member

bsipocz commented Jul 7, 2023

pip install -U --pre astroquery should get you the new module :)

@ManonMarchand
Copy link
Member Author

Awesome! Thanks!

@ManonMarchand ManonMarchand deleted the rename-cds-into-mocserver branch September 5, 2023 11:58
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

2 participants