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.

Sign up for GitHub

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

Jump to bottom

Replace tmpdir with tmp_path #1095

Merged
merged 3 commits into from
Nov 14, 2023
Merged

Replace tmpdir with tmp_path #1095

merged 3 commits into from
Nov 14, 2023

Conversation

rosteen
Copy link
Contributor

@rosteen rosteen commented Nov 14, 2023

Closes #1039 .

@rosteen rosteen changed the title No more tmpdir Replace tmpdir with tmp_path Nov 14, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 14, 2023 •
edited
Loading

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (5098f23) 70.73% compared to head (1bdb79e) 70.73%.

Files Patch % Lines
specutils/io/default_loaders/tests/test_apogee.py 50.00% 3 Missing ⚠️
...utils/io/default_loaders/tests/test_jwst_reader.py 92.00% 2 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1095   +/-   ##
=======================================
  Coverage   70.73%   70.73%           
=======================================
  Files          64       64           
  Lines        4483     4483           
=======================================
  Hits         3171     3171           
  Misses       1312     1312

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rosteen rosteen requested a review from pllim November 14, 2023 21:38
@rosteen rosteen added this to the v1.x milestone Nov 14, 2023
pllim
pllim reviewed Nov 14, 2023
View reviewed changes
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Seems uncontroversial enough though maybe one of the tests does not need tmp_path in the first place.

specutils/io/default_loaders/tests/test_jwst_reader.py Outdated Show resolved Hide resolved
@rosteen @pllim
Removed unused tmp_path
1bdb79e 
Co-authored-by: P. L. Lim <[email protected]>
pllim
pllim reviewed Nov 14, 2023
View reviewed changes
specutils/io/default_loaders/tests/test_jwst_reader.py
@@ -366,7 +366,7 @@ def generate_s3d_wcs():


@pytest.fixture()
def tmp_asdf(tmpdir):
def tmp_asdf():
Copy link
Member

Choose a reason for hiding this comment

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

What hurts my head is tmp_asdf itself is a fixture but it took in tmpdir fixture. What was it trying to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a mystery 😕

pllim
pllim approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

LGTM now as long as CI agrees with me. Thanks!

@pllim
Copy link
Member

pllim commented Nov 14, 2023

I will let you merge.

@rosteen rosteen merged commit 8707443 into astropy:main Nov 14, 2023
11 checks passed
rosteen added a commit to rosteen/specutils that referenced this pull request Nov 22, 2023
@rosteen @pllim
Replace tmpdir with tmp_path (astropy#1095)
f92efdc 
* Replace tmpdir with tmp_path

* Fix path construction

* Removed unused tmp_path

Co-authored-by: P. L. Lim <[email protected]>

---------

Co-authored-by: P. L. Lim <[email protected]>
rosteen added a commit that referenced this pull request Nov 22, 2023
@pllim @rosteen
TST: Fix devdeps and clean up matrix (#1111)
53af673 
* TST: Pin casa-formats-io in oldest-deps
job because it is upgrading numpy but we do not want that to happen.

* Replace tmpdir with tmp_path (#1095)

* Replace tmpdir with tmp_path

* Fix path construction

* Removed unused tmp_path

Co-authored-by: P. L. Lim <[email protected]>

---------

Co-authored-by: P. L. Lim <[email protected]>

* Fix devdeps and clean up matrix
and stuff

* Use astropy>=5.3 for doctest

* Add tox back in

---------

Co-authored-by: Ricky O'Steen <[email protected]>
Co-authored-by: Ricky O'Steen <[email protected]>
rosteen added a commit to rosteen/specutils that referenced this pull request Nov 29, 2023
@pllim @rosteen
TST: Fix devdeps and clean up matrix (astropy#1111)
2ccdd6c 
* TST: Pin casa-formats-io in oldest-deps
job because it is upgrading numpy but we do not want that to happen.

* Replace tmpdir with tmp_path (astropy#1095)

* Replace tmpdir with tmp_path

* Fix path construction

* Removed unused tmp_path

Co-authored-by: P. L. Lim <[email protected]>

---------

Co-authored-by: P. L. Lim <[email protected]>

* Fix devdeps and clean up matrix
and stuff

* Use astropy>=5.3 for doctest

* Add tox back in

---------

Co-authored-by: Ricky O'Steen <[email protected]>
Co-authored-by: Ricky O'Steen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pllim pllim pllim approved these changes

@eteq eteq Awaiting requested review from eteq eteq is a code owner

@nmearl nmearl Awaiting requested review from nmearl

@keflavich keflavich Awaiting requested review from keflavich keflavich is a code owner

Assignees
No one assigned
Labels
testing
Projects
None yet
Milestone
v1.x
Development

Successfully merging this pull request may close these issues.

TST: Replace tmpdir with tmp_path in pytest
2 participants
@rosteen @pllim