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

Deprecate astroquery/utils/download_file_list.py #2247

Merged
merged 2 commits into from
Dec 17, 2021

Conversation

eerovaher
Copy link
Member

@eerovaher eerovaher commented Dec 13, 2021

The contents of this file were not being used anywhere, so it should be safe to remove even without deprecating first.

EDIT: This pull request will deprecate the module, it will be removed later.

@codecov
Copy link

codecov bot commented Dec 13, 2021

Codecov Report

Merging #2247 (a96365f) into main (8a60cd2) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2247      +/-   ##
==========================================
+ Coverage   62.38%   62.40%   +0.02%     
==========================================
  Files         131      131              
  Lines       16775    16777       +2     
==========================================
+ Hits        10465    10470       +5     
+ Misses       6310     6307       -3     
Impacted Files Coverage Δ
astroquery/utils/download_file_list.py 28.12% <100.00%> (+7.15%) ⬆️

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 8a60cd2...a96365f. Read the comment docs.

@keflavich
Copy link
Contributor

This is a public function, so technically someone could be using it standalone. However, I don't think it's very likely since we didn't document it well.

I'm ambivalent about removing this: since we don't use it, it's fine to remove, but it looks like something that could be useful. Within astroquery, though, we should stick to the other extensive download tools we've built.

@bsipocz
Copy link
Member

bsipocz commented Dec 13, 2021

I'm cutting 0.4.5 this week, let's have this milestone for 4.0, we can merge this when we know for sure that it's the next one coming (I wish it was, but I cannot say it for sure as we need to do a lot of cleanup work for it).

In the meantime I'm not opposed to have this merged whenever, of you decorate it as deprecation.

@eerovaher
Copy link
Member Author

Looks like deprecation is preferred over immediate removal. The module only provides one public function (__all__ = ['download_list_of_fitsfiles']), so I will deprecate that.

This commit deprecates the function `download_list_of_fitsfiles()`, and
because that is the only public function defined by the aforementioned
file then the whole file becomes deprecated.
@eerovaher eerovaher changed the title Remove astroquery/utils/download_file_list.py Deprecate astroquery/utils/download_file_list.py Dec 14, 2021
@bsipocz bsipocz added the utils label Dec 17, 2021
@bsipocz bsipocz added this to the v0.4.5 milestone Dec 17, 2021
@bsipocz
Copy link
Member

bsipocz commented Dec 17, 2021

Thanks, this is now uncontroversial.

@bsipocz bsipocz merged commit 17c79f9 into astropy:main Dec 17, 2021
@eerovaher eerovaher deleted the rm-download-file-list branch December 27, 2021 14:23
syed-gilani pushed a commit to syed-gilani/astroquery that referenced this pull request Mar 11, 2022
burnout87 pushed a commit to oda-hub/astroquery that referenced this pull request May 10, 2022
mhsarmiento pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request May 24, 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.

None yet

3 participants