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

grass.jupyter: Support pathlib.Path in TimeSeriesMap.save #2453

Merged
merged 3 commits into from
Feb 15, 2023

Conversation

wenzeslaus
Copy link
Member

@wenzeslaus wenzeslaus commented Jun 20, 2022

  • API: TimeSeriesMap.save now supports pathlib.Path objects (and strings as before).
  • tests: Use tmp_path, not current dir. Files generated by the tests should use tmp_path (or other pytest tmp dir ways), not the current directory.

@wenzeslaus wenzeslaus marked this pull request as draft June 20, 2022 18:56
@wenzeslaus wenzeslaus added this to the 8.4.0 milestone Jul 25, 2022
@wenzeslaus wenzeslaus added the Python Related code is in Python label Jul 26, 2022
@wenzeslaus wenzeslaus force-pushed the use-tmp-path-for-pytest-results branch from 36966dc to 77b73c9 Compare February 10, 2023 19:25
@wenzeslaus wenzeslaus marked this pull request as ready for review February 10, 2023 19:26
@wenzeslaus wenzeslaus added the bug Something isn't working label Feb 10, 2023
Copy link
Contributor

@chaedri chaedri left a comment

Choose a reason for hiding this comment

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

Looks good to me! Does writing to the current directory cause tests to fail?

I tried to figure out why the pytest check is failing (AttributeError: 'PosixPath' object has no attribute 'endswith') but I'm not sure.

@wenzeslaus wenzeslaus force-pushed the use-tmp-path-for-pytest-results branch from 77b73c9 to 50129b2 Compare February 13, 2023 16:35
@wenzeslaus wenzeslaus changed the title tests: Use tmp_path, not current dir grass.jupyter: Support pathlib.Path in TimeSeriesMap.save Feb 13, 2023
@wenzeslaus
Copy link
Member Author

The code should actually support both string and Path objects, but it supported only strings, so the new text code broke it. This PR now fixes the code.

Writing to the the current works (most of the time), but a file is left in that directory while using pytest tmp dir, the files are managed by pytest and in one place. (They are accessible after the test finishes and are deleted time to time.)

@wenzeslaus wenzeslaus force-pushed the use-tmp-path-for-pytest-results branch from 50129b2 to 8042302 Compare February 14, 2023 16:07
Files generated by the tests should use tmp_path (or other pytest tmp dir ways), not the current directory.
…e suffix (extension) instead of manipulating strings. PIL.Image supports Path for 8 years.
@wenzeslaus wenzeslaus force-pushed the use-tmp-path-for-pytest-results branch from 8042302 to 3ca1eb2 Compare February 14, 2023 18:58
Copy link
Contributor

@chaedri chaedri left a comment

Choose a reason for hiding this comment

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

I understand now. Thanks! Looks good to me.

@wenzeslaus wenzeslaus merged commit adf4d3e into OSGeo:main Feb 15, 2023
@wenzeslaus wenzeslaus deleted the use-tmp-path-for-pytest-results branch February 15, 2023 20:13
ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023
When the save method receives a path, ensure the path is pathlib.Path, then use its functions to detect file suffix (extension) instead of manipulating strings. PIL.Image supports Path for 8 years, so we can support that and get the string support by conversion to Path.

In tests, use tmp_path, not current dir. Files generated by the tests should use tmp_path (or other pytest tmp dir ways), not the current directory.
neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023
When the save method receives a path, ensure the path is pathlib.Path, then use its functions to detect file suffix (extension) instead of manipulating strings. PIL.Image supports Path for 8 years, so we can support that and get the string support by conversion to Path.

In tests, use tmp_path, not current dir. Files generated by the tests should use tmp_path (or other pytest tmp dir ways), not the current directory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Python Related code is in Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants