biocore / biom-format

The Biological Observation Matrix (BIOM) Format Project
http://biom-format.org
Other
89 stars 95 forks source link

Windows support #951

Closed mataton closed 4 months ago

mataton commented 4 months ago

A continuation of PR #949, this PR adds support for Windows.

All issues revolve around two things: 1) File path separators differing between operating systems. / vs. \ 2) Temporary file handling differing between operating systems.

The first issue is solved with a straightforward fix using os.join() and os.path.sep.

The second issue is more complex. In Windows, the name of a NamedTemporaryFile object cannot be used to open the file a second time, while the named temporary file is still open. See this SO, and the Python docs. The strategy is to use the delete=False parameter to prevent the temporary file from being deleted when it is closed, and then manually delete the file when the test is done using it.

mataton commented 4 months ago

@wasade Essentially it looks like there are two scenarios. One where all the tests in whichever test.py file require the temporary files. In this case, the setUp/tearDown solution I feel is best. This is true in test_add_metadata.py, test_summarize_table.py, and test_table_converter.py.

The other scenario is that not all of the tests within the test.py require the temporary files, so using setUp/tearDown framework doesn't make sense here. The current solution in those files I think might be the most appropriate, considering my previous comments.

wasade commented 4 months ago

Thanks, @mataton. I still think we do want unlink in tearDown as that ensures it runs even in the event of an exception, does that make sense? It's relatively minor here though as these are still going into TMPDIR

mataton commented 4 months ago

@wasade Your suggested changes make sense, and there was actually already a to_remove = [] in the setUp functions that I didn't notice previously, which I used. Just had to add the proper tearDown to a couple files. It should be good now.

wasade commented 4 months ago

Will merge once aarch64 completes

wasade commented 4 months ago

...thank you @mataton, this is super helpful!!! One last request, could you add a changelog note?

mataton commented 4 months ago

Happy to contribute!