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

Atomic: change http->https, and do a lot of refactoring #2088

Merged
merged 4 commits into from Jun 21, 2021

Conversation

keflavich
Copy link
Contributor

Bugfix for #2065

@pep8speaks
Copy link

pep8speaks commented Jun 20, 2021

Hello @keflavich! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-06-21 19:41:27 UTC

@codecov
Copy link

codecov bot commented Jun 20, 2021

Codecov Report

Merging #2088 (3e049f5) into main (0de7f0e) will decrease coverage by 0.00%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2088      +/-   ##
==========================================
- Coverage   66.56%   66.56%   -0.01%     
==========================================
  Files         407      407              
  Lines       27647    27652       +5     
==========================================
+ Hits        18403    18406       +3     
- Misses       9244     9246       +2     
Impacted Files Coverage Δ
astroquery/atomic/__init__.py 91.07% <ø> (ø)
astroquery/atomic/core.py 37.90% <33.33%> (+0.92%) ⬆️
astroquery/gaia/core.py 61.59% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0de7f0e...3e049f5. Read the comment docs.

@bsipocz bsipocz added this to the v0.4.3 milestone Jun 21, 2021
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.

Overall it looks good. I have added a few minor comments.
Do add a changelog entry though.

astroquery/atomic/core.py Outdated Show resolved Hide resolved
astroquery/atomic/core.py Outdated Show resolved Hide resolved
astroquery/atomic/core.py Outdated Show resolved Hide resolved
astroquery/atomic/core.py Outdated Show resolved Hide resolved
@keflavich keflavich mentioned this pull request Jun 21, 2021
2 tasks
Comment on lines -42 to -43
Service fixes and enhancements
------------------------------
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bsipocz note that this was a redundant header - we might have to remove this in other PRs too

Copy link
Member

Choose a reason for hiding this comment

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

oh, do remove it in one PR only, the other may end up in a conflict otherwise.

@bsipocz bsipocz linked an issue Jun 21, 2021 that may be closed by this pull request
@bsipocz
Copy link
Member

bsipocz commented Jun 21, 2021

(@keflavich - in the future, do use one of the "magic" words before the issue number in PR descriptions, that case they got linked and closed automatically)

https://docs.github.com/en/issues/tracking-your-work-with-issues/creating-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

@keflavich keflavich merged commit 7609419 into astropy:main Jun 21, 2021
@keflavich keflavich deleted the atomic_fix_2065 branch June 21, 2021 20:18
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.

Astroquery AtomicLineList query error
3 participants