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: undefined variable in timezone logic #370

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

m-richards
Copy link
Member

@m-richards m-richards commented Mar 3, 2024

I don't know how I missed this in #253, but there's a logical error if an exception is thrown in this block.

I'm not quite sure how to test this - I only noticed the error in my geopandas dev environment where I had pyogrio 0.7.2 with pandas dev - so I was getting the exception fixedby #348 (not part of 0.7.2), not a future error due to mixed time zones - which is what this try except was originally for.

Copy link
Member

@brendan-ward brendan-ward left a comment

Choose a reason for hiding this comment

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

Thanks for catching this!

Going to merge now so as to avoid this remaining undefined. However, does this also indicate that instead of catching all exceptions (via except Exception) that there should be different handling for the future error of mixed time zones?

@brendan-ward brendan-ward merged commit 16db572 into geopandas:main Mar 5, 2024
19 checks passed
@m-richards
Copy link
Member Author

Thanks for catching this!

Going to merge now so as to avoid this remaining undefined. However, does this also indicate that instead of catching all exceptions (via except Exception) that there should be different handling for the future error of mixed time zones?

I can have another look but I didn't think this had been done yet - the error I saw was for expiring errors="ignore" - which now throws an assertionerror, I don't imagine that convention would be copied

@m-richards m-richards deleted the fix_timezone_bug branch March 5, 2024 02:27
@martinfleis
Copy link
Member

This is now showing up in geopandas/geopandas#3223. Might be worth trying to get a version with this out sooner than later to avoid issues when testing against geopandas main (and to get green CI in geopandas).

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.

3 participants