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

Bug fixes to arcgisimage #598

Merged
merged 7 commits into from
Feb 14, 2024
Merged

Bug fixes to arcgisimage #598

merged 7 commits into from
Feb 14, 2024

Conversation

nitram96
Copy link
Contributor

@nitram96 nitram96 commented Feb 8, 2024

fixed bugs with using cachedir in arcgisimage.
cachedir is no longer ignored if verbose is false
if a cached image is found, now correctly shows it using self.imshow()
changed to use os.path.join to join cachedir and filename, so a backslash is no longer required at the end of cachedir

…ignored if verbose is false, if a cached image is found now correctly shows it using self.imshow(), now uses os.path.join to join cachedir and filename, so a backslash is no longer required at the end of cachedir
@nitram96 nitram96 changed the title Develop Bug fixes to arcgisimage Feb 8, 2024
@molinav
Copy link
Member

molinav commented Feb 8, 2024

Thanks, @nitram96! It is clear that your changes are correct, let's wait for the workflows to finish and I merge it.

Hopefully I will find some time to increase the coverage of the library. With more tests I would not have overlooked this bug.

@molinav
Copy link
Member

molinav commented Feb 12, 2024

@nitram96 Sorry for the delay, everything looks fine, but I would like to add a couple of unit tests before merging. I hope to find some time within the next 2 days, and then most likely I will tag as basemap release 1.4.1 after the merge, because having such method broken in the latest stable is not nice.

@molinav
Copy link
Member

molinav commented Feb 14, 2024

@nitram96 I took the opportunity that you opened the PR to fix a bit more the Basemap.arcgisimage method:

  • The method code is now compliant with the most basic flake8 and pylint requirements.
  • The docstring has been reformated so that it will get rendered correctly by Sphinx with the current furo theme.
  • The cache directory should be created on the fly if saving a cache image for the first time and cachedir is a string corresponding to a non-existing directory. This code block was located at the wrong place, so I moved it accordingly.
  • I added a couple of tests for Basemap.arcgisimage using the cachedir argument, one with an existing directory and another with a still non-existing directory.

@molinav
Copy link
Member

molinav commented Feb 14, 2024

All seems in order, so I am merging. Thanks again, @nitram96!

@molinav molinav merged commit 40db9b3 into matplotlib:develop Feb 14, 2024
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