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

Alfalfa documentation clean up #1941

Merged
merged 3 commits into from
May 23, 2022

Conversation

tinuademargaret
Copy link
Contributor

For some reason querying spectrumAlfalfa.get_spectrum(agc) times out. I'm yet to figure out why so I just commented out that part of the doc for now.

@astropy-bot astropy-bot bot added the utils label Jan 13, 2021
@tinuademargaret tinuademargaret changed the title Alfaalfa documentation clean up Alfalfa documentation clean up Jan 13, 2021
@tinuademargaret tinuademargaret marked this pull request as draft January 13, 2021 15:09
@bsipocz bsipocz added alfalfa and removed utils labels Mar 26, 2022
@bsipocz bsipocz added this to the v0.4.7 milestone May 16, 2022
@bsipocz bsipocz marked this pull request as ready for review May 16, 2022 19:28
@bsipocz bsipocz requested a review from keflavich May 16, 2022 19:29
@bsipocz
Copy link
Member

bsipocz commented May 16, 2022

@keflavich - please do a review if you're OK with getting rid of the iterator approach to work around the byte/str issues.

@codecov
Copy link

codecov bot commented May 16, 2022

Codecov Report

Merging #1941 (8f330cf) into main (b2bd214) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1941   +/-   ##
=======================================
  Coverage   62.96%   62.96%           
=======================================
  Files         133      133           
  Lines       17287    17287           
=======================================
  Hits        10885    10885           
  Misses       6402     6402           
Impacted Files Coverage Δ
astroquery/alfalfa/core.py 96.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

lgtm, but let's hold off merging this for a few days while I wait for a reply on whether this service is still active at all

@bsipocz
Copy link
Member

bsipocz commented May 23, 2022

@keflavich - did they get back to you?

@keflavich
Copy link
Contributor

No, no reply. I think we need to add a deprecation tag to this whole module - it looks like the server that hosted the data is down and no mirror has been made. I guess we can merge this PR but open a new issue that this module connects to a remote service that is presently unavailable. I'd like the formal confirmation that it's never coming back before we delete the module, though.

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

3 participants