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

Refactor astroquery/utils/system_tools.py and its tests #2306

Merged
merged 2 commits into from
Feb 24, 2022

Conversation

eerovaher
Copy link
Member

See the commit messages for details.

1. There is no need to check for the availability of `gzip` because it
   is in the Python standard library.
2. There is no need to write code for managing temporary directories
   because `pytest` provides the `tmp_path` fixture.
3. A separate test for checking the existance of a file produced by the
   tests is not needed because if it does not exist the following tests
   fail anyways.
All imports are now done at modul level and unused code has been
removed.
@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #2306 (2920b77) into main (f99e1ba) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2306   +/-   ##
=======================================
  Coverage   62.84%   62.84%           
=======================================
  Files         130      130           
  Lines       16841    16837    -4     
=======================================
- Hits        10584    10582    -2     
+ Misses       6257     6255    -2     
Impacted Files Coverage Δ
astroquery/utils/system_tools.py 83.33% <100.00%> (+8.33%) ⬆️

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 f99e1ba...2920b77. Read the comment docs.

@bsipocz
Copy link
Member

bsipocz commented Feb 24, 2022

I recall some issues around non-availablity of gzip on windows, thus this whole workaround was added in #1613? It was done after we dropped python2 support though

@pllim - do you see any issue with this PR?

@pllim
Copy link
Member

pllim commented Feb 24, 2022

I can see gzip on Windows with Python 3.8. If it passes in your Windows CI, probably ok?

@bsipocz
Copy link
Member

bsipocz commented Feb 24, 2022

OK, thanks for confirming. Let's try to see this then, thanks @eerovaher for the cleanup.

@bsipocz bsipocz merged commit 843f9da into astropy:main Feb 24, 2022
@eerovaher
Copy link
Member Author

I'll try to clear any confusion there may be about the availability of gzip on different platforms. There exists a program called gzip which has nothing to do with Python, and which is not guaranteed to be installed by all users. Before #1613 astroquery tried to use that program, but its availability had to be checked. What #1613 did was it replaced the external gzip with the Python standard library module also called gzip, which can be safely assumed to be available for all astroquery users without any need to check its presence.

@bsipocz
Copy link
Member

bsipocz commented Feb 24, 2022

Yes, I suppose part of the issues came with the fact that the standalone, UNIX gzip could deal with the fits.Z files, but it broke when we switched. Or something else, I can't recall and some of that may be lost in discussions on slack, but it certainly raised as an issue during the precious PR and thus we ended up with the extra checks.

@eerovaher eerovaher deleted the refactor-systools branch March 8, 2022 17:30
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.

3 participants