MNT: Run PTH flake test in prep for supporting pathlib (wcs)#16936
MNT: Run PTH flake test in prep for supporting pathlib (wcs)#16936neutrinoceros wants to merge 2 commits intoastropy:mainfrom
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
889e399 to
ea7c99d
Compare
ea7c99d to
b61e873
Compare
5ff91c3 to
667e602
Compare
667e602 to
afdf630
Compare
|
If I understand correctly this is waiting on a new |
470e71b to
8579ab1
Compare
|
@neutrinoceros - FYI I've just triggered a 1.2.0 release of extension-helpers: https://github.com/astropy/extension-helpers/releases/tag/v1.2.0 - build ongoing |
|
Thanks @astrofrog ! I took advantage of it to simplify this PR |
|
whoops, I neglected that we cannot trivially bump build time dependencies without breaking exotic archs, so let me postpone this last commit to a follow up PR. update: this is #17193 |
8579ab1 to
15d5ac8
Compare
|
We're hours away from astropy 7.0's feature freeze and this shouldn't be blocking in any way, so I'll bump the milestone to clear the backlog. If this happened to be merged today anyway, please revert the milestone. |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
| def test_tpv_ctype_tan(): | ||
| sip_header = fits.Header.fromstring( | ||
| get_pkg_data_contents(os.path.join("data", "siponly.hdr"), encoding="binary") | ||
| get_pkg_data_contents(Path("data") / "siponly.hdr", encoding="binary") |
There was a problem hiding this comment.
maybe consolidate the Path("data") here and below into a variable?
Actually, make it a pytest fixture! It's used all over this file.
There was a problem hiding this comment.
Actually! put it in wcs/tests/conftest.py then it can be used across all the wcs/tests.
There was a problem hiding this comment.
Hum, I'm not convinced that such a small fixture is worth adding an indirection to all tests that would use it.
15d5ac8 to
d2f5173
Compare
Co-authored-by: Mridul Seth <git@mriduls.com>
0d4917f to
a58249e
Compare
|
@nstarman thank you for picking this up. I should have addressed all your points. |
|
@neutrinoceros more test coverage needed? |
|
done |
nstarman
left a comment
There was a problem hiding this comment.
Given the followup to un-stringify some of these paths, LGTM! Thanks @neutrinoceros.
Whom else needs to approve?
There was a problem hiding this comment.
Hmm low level API is bound by https://github.com/astropy/astropy-APEs/blob/main/APE14.rst . Does GWCS need to buy in also? cc @astrofrog @Cadair @nden
|
Given the recent revert of PTH changes, let's not do this for now. Thanks anyway! |
Description
Extracted from #16060. Ref #16924