Skip to content

Conversation

@neutrinoceros
Copy link
Contributor

This is a small cleanup I stumbled upon. My original motivation was more to cleanup the test than to add a feature, strange how things work out sometimes.

@neutrinoceros neutrinoceros marked this pull request as ready for review August 15, 2025 11:01
@neutrinoceros neutrinoceros changed the title ENH: add support for loading Dataset a pathlib.Path ENH: add support for loading Dataset from a pathlib.Path Aug 16, 2025
@VChristiaens
Copy link
Contributor

Thanks for the PR @neutrinoceros. There seems to be a second part to this PR that is not really captured in the title if I'm not misatken. Could you expand on what's now done in the test_dataset.py test? In particular, is removing the temporary written file not needed any more?

@VChristiaens
Copy link
Contributor

Hi @neutrinoceros, just following up quickly on this PR since it's pretty much mergeable. Before accepting it, I was just wondering what was the motivation for removing the line that remove the temporary file that is created and read in the test? I'd naively expect it to be a good thing to remove the unnecessary file created by the test - but maybe I'm missing something? Thanks again for all your contributions!

@neutrinoceros
Copy link
Contributor Author

Sorry, this fell through the cracks of my backlog. I changed the test to use the native tmp_path pytest fixture, which handles resource destruction (i.e. cleaning up the temporary directory) itself, and in a more robust fashion: it'll clean it up even if the test fails on an assertion, while the pre-existing implementation wouldn't

@VChristiaens VChristiaens merged commit 03a7621 into vortex-exoplanet:master Oct 13, 2025
8 checks passed
@VChristiaens
Copy link
Contributor

That's good to know - thanks a lot for this addition and for the clarification!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants