Skip to content

fix: parse timezone-aware datetime strings as UTC consistently across backends#2166

Merged
MarcoGorelli merged 15 commits intonarwhals-dev:mainfrom
MarcoGorelli:parsing-tz-aware
Mar 7, 2025
Merged

fix: parse timezone-aware datetime strings as UTC consistently across backends#2166
MarcoGorelli merged 15 commits intonarwhals-dev:mainfrom
MarcoGorelli:parsing-tz-aware

Conversation

@MarcoGorelli
Copy link
Member

What type of PR is this? (check all applicable)

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

  • Related issue #<issue number>
  • Closes #<issue number>

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below

@MarcoGorelli MarcoGorelli marked this pull request as ready for review March 7, 2025 20:47
if "pandas" in str(constructor) and PANDAS_VERSION < (1,):
# "Cannot pass a tz argument when parsing strings with timezone information."
pytest.skip()
if "pyarrow_table" in str(constructor) and is_windows():
Copy link
Member

Choose a reason for hiding this comment

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

Hate to be the windows guy but ...

narwhals/tests/utils.py

Lines 170 to 172 in caaa840

def is_pyarrow_windows_no_tzdata(constructor: Constructor, /) -> bool:
"""Skip test on Windows when the tz database is not configured."""
return "pyarrow" in str(constructor) and is_windows() and not windows_has_tzdata()

Copy link
Member

Choose a reason for hiding this comment

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

I think that util needs updating to use "pyarrow_table" actually

My bad 🤦

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Oh it turns out I accidentally had the right thing lol https://github.com/narwhals-dev/narwhals/actions/runs/13730338881/job/38405984916?pr=2166

Double my bad 🤦🤦

needs reverting (fdb2f48)

Copy link
Member

Choose a reason for hiding this comment

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

@MarcoGorelli fyi I un-resolved this

@@ -169,4 +169,6 @@ def windows_has_tzdata() -> bool: # pragma: no cover

def is_pyarrow_windows_no_tzdata(constructor: Constructor, /) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

Truly unfortunate name I settled on here 😭

Copy link
Member

@dangotbanned dangotbanned left a comment

Choose a reason for hiding this comment

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

Thanks @MarcoGorelli

@dangotbanned dangotbanned added bug: incorrect result Something isn't working fix labels Mar 7, 2025
@MarcoGorelli
Copy link
Member Author

thanks for your review!

@MarcoGorelli MarcoGorelli merged commit 1c512e2 into narwhals-dev:main Mar 7, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug: incorrect result Something isn't working fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants