Repository navigation
Moved astropy.vo.samp to astropy.samp - #6213
Conversation
|
Wait a minute... Are you removing |
|
@pllim - But conesearch got moved out, not just shuffled around. |
pllim
left a comment
pllim
left a comment
There was a problem hiding this comment.
I think completely removing vo without deprecation period is too drastic. I propose we deprecate vo.samp first in v2; i.e., anyone calling vo.samp will get a deprecation warning and vo.samp is actually a wrapper around samp. Then we can remove the whole vo sub-package in v3. What do you think?
| @@ -62,7 +62,7 @@ | |||
|
|
|||
| [vo.samp] | |||
There was a problem hiding this comment.
Don't you need a new [samp] section too?
|
@pllim - no I've only removed |
|
@astrofrog , moving doc only is probably okay, but the diff also shows you moved the codes and deleted some cone search stuff. |
|
@pllim - I just moved the files up one directory, and In [2]: from astropy.vo.samp import SAMPClient
WARNING: AstropyDeprecationWarning: The astropy.vo.samp module has now been moved to astropy.samp [astropy.vo.samp]
|
|
@astrofrog , okay, I was just freaking out of all the cone search doc deletion. 😅 It just seems confusing to a user that they can still use EDIT: Would be nice to also retain the "Other third-party Python packages..." text until we completely get rid of Also need to update the "stability" listing. |
|
@pllim - I've undeleted the vo docs! |
|
Thanks! Still need a change log and edits to |
|
@pllim - done! |
|
Before we merge this, just a reminder that I'd like to make sure @eteq is happy with this |
|
|
||
| - The `astropy.vo.samp` package no longer supports SSL. [#6201] | ||
|
|
||
| - The `astropy.vo.samp` package has been moved to `astropy.samp`. [#6213] |
There was a problem hiding this comment.
Nitpick: This entry and the one for #6201 above are a bit confusing. Maybe merge them into a single entry and reword in a more coherent manner?
| <tr> | ||
| <td> | ||
| astropy.vo.samp | ||
| astropy.samp |
There was a problem hiding this comment.
Maybe it is clearer to state in the description that it was renamed from astropy.vo.samp in v2.0? Other entries have info like that (when added, etc).
|
Added @eteq as a reviewer, as he needs to be happy with this move :) |
eteq
left a comment
eteq
left a comment
There was a problem hiding this comment.
One very minor comment, which could be changed at merge since it's just a doc change. Once the tests are done I think this is all set.
| The ``astropy.vo`` module has been deprecated and will be removed | ||
| in a future version. The SAMP functionality has been moved | ||
| to ``astropy.samp``, and the conesearch functionality has been | ||
| moved to `Astroquery <http://astroquery.readthedocs.io>`_. |
There was a problem hiding this comment.
Shouldn't this also mention that this is the "classic" API?
There was a problem hiding this comment.
@eteq , are you talking about Cone Search or SAMP's "classic" API?
There was a problem hiding this comment.
Oops! Sorry, misunderstanding: I thought the what's new page was saying that there "classic" API is in astropy, but I get now that that's referencing the astroquery version. So no change needed.
bsipocz
added
the
zzz 💤 merge-when-ci-passes
| [#5558, #5904] | ||
|
|
||
| - The `astropy.vo.samp` package no longer supports SSL. [#6201] | ||
| - The `astropy.vo.samp` package has been moved to `astropy.samp`, and no |
There was a problem hiding this comment.
use double backticks to make sphinx happy
|
Looks like the tests passed except for that one backtick issue that I fixed directly in this branch. Merging! |
MSeifert04
removed
the
zzz 💤 merge-when-ci-passes
5 participants
@taldcroft and @mhvk agreed to this plan in #6201
@eteq @kelle @pllim - any objections to this?
@pllim - I actually ended up just removing
docs/voand adding the note you had added about astroquery to the what's new instead. Does that seem ok?