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

Thin out & refactor astroquery.alma.utils #2331

Merged
merged 2 commits into from
Oct 2, 2022

Conversation

keflavich
Copy link
Contributor

WIP to fix #1794 (I still need to do more testing and probably import cleanup).

The old finder chart code resides here:
https://gist.github.com/keflavich/165b10ce09b68fb3deda50df85cf2ce8
and needs to be reworked to avoid aplpy & pyregion.

@pep8speaks
Copy link

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

Line 67:9: E225 missing whitespace around operator
Line 69:11: E225 missing whitespace around operator
Line 82:13: E265 block comment should start with '# '

@bsipocz bsipocz added this to the v0.4.7 milestone Mar 22, 2022
@bsipocz bsipocz added the alma label Mar 22, 2022
@bsipocz
Copy link
Member

bsipocz commented Mar 22, 2022

@keflavich - If you would separate the function removals from the refactoring, we can ship that in 0.4.6? Or shall just add a deprecation decorator on those in 0.4.6 (though they are not really compatible with the alma API any more, so probably there are no users out there), and we then remove them in 0.4.7?

@bsipocz bsipocz force-pushed the astroquery_utils_rm_finderchart branch from 075b78f to 0ae5dad Compare October 2, 2022 01:41
@bsipocz
Copy link
Member

bsipocz commented Oct 2, 2022

I've rebased and fixed this up to remove the now deprecated functions.

@codecov
Copy link

codecov bot commented Oct 2, 2022

Codecov Report

Merging #2331 (0ae5dad) into main (4b4af55) will increase coverage by 0.36%.
The diff coverage is 10.00%.

@@            Coverage Diff             @@
##             main    #2331      +/-   ##
==========================================
+ Coverage   63.63%   63.99%   +0.36%     
==========================================
  Files         132      132              
  Lines       17092    16969     -123     
==========================================
- Hits        10876    10859      -17     
+ Misses       6216     6110     -106     
Impacted Files Coverage Δ
astroquery/alma/utils.py 32.43% <10.00%> (+14.30%) ⬆️

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

@bsipocz
Copy link
Member

bsipocz commented Oct 2, 2022

Tests are passing, so I go ahead and merge this.

@bsipocz bsipocz merged commit b0d9f5f into astropy:main Oct 2, 2022
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.

Fix utils functions in the alma module
3 participants