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

fix(python): Don't ignore timezones in list of dicts constructor #14211

Merged
merged 8 commits into from
Jul 24, 2024

Conversation

deanm0000
Copy link
Collaborator

@deanm0000 deanm0000 commented Feb 2, 2024

fixes:

closes #12151
closes #12638
closes #7620

I initially was hesitant to touch this issue in python thinking it was in rust but I see that sequence_to_pyseries is handling timezones for a dict of lists so the issue was never in rust, it was just lack of implementation. Unfortunately unless we want to turn a list of dicts into a dict of lists in python, it fixes this through a post PyDataFrame cast

@github-actions github-actions bot added fix Bug fix python Related to Python Polars labels Feb 2, 2024
@deanm0000 deanm0000 changed the title fix(python): timezones in list of dicts constructor aren't ignored fix(python): don't ignore timezones in list of dicts constructor Feb 2, 2024
@MarcoGorelli
Copy link
Collaborator

since #16828 has been merged there's some conflicts - do you have time to fix them up, and update the logic accordingly? I think it should be much simpler now, just use convert_time_zone

sorry this took a while to get round to

@deanm0000
Copy link
Collaborator Author

It'll probably be awhile before I'll be able to make the time to get into it.

@stinodego stinodego requested a review from reswqa as a code owner June 28, 2024 17:53
Copy link

codecov bot commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.51%. Comparing base (383c48a) to head (4049887).
Report is 133 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14211      +/-   ##
==========================================
+ Coverage   80.47%   80.51%   +0.03%     
==========================================
  Files        1483     1504      +21     
  Lines      195118   197156    +2038     
  Branches     2778     2806      +28     
==========================================
+ Hits       157019   158733    +1714     
- Misses      37588    37902     +314     
- Partials      511      521      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MarcoGorelli MarcoGorelli marked this pull request as draft July 9, 2024 16:02
Copy link
Collaborator

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks @deanm0000 , sorry it a while to get to - I think the logic can now be considerably simplified. I've added some commits, fancy taking a look to see if they make sense to you?

@MarcoGorelli MarcoGorelli marked this pull request as ready for review July 9, 2024 16:57
@MarcoGorelli
Copy link
Collaborator

@alexander-beedie fancy taking a look at this one?

@MarcoGorelli MarcoGorelli changed the title fix(python): don't ignore timezones in list of dicts constructor fix(python): Don't ignore timezones in list of dicts constructor Jul 21, 2024
@alexander-beedie
Copy link
Collaborator

@alexander-beedie fancy taking a look at this one?

Looking ✌️

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Jul 23, 2024

@deanm0000: I want to push a few small updates to the branch to streamline this a bit further, but despite "Maintainers are allowed to edit this pull request" being checked, I can't seem to push to the branch; could you invite/enable me directly? :)

remote: Permission to deanm0000/polars.git denied to alexander-beedie. 
unable to access 'https://github.com/deanm0000/polars.git/': 

@deanm0000
Copy link
Collaborator Author

@alexander-beedie I don't know how. I tried googling a bit and it wasn't obvious.

@MarcoGorelli you put commits in. Any tips here?

@alexander-beedie
Copy link
Collaborator

@alexander-beedie I don't know how. I tried googling a bit and it wasn't obvious.

Think this should do it? 🤔

@MarcoGorelli
Copy link
Collaborator

hey @alexander-beedie - I don't think extra permissions are required, all you should need to do is:

git remote add deanm0000 git@github.com:deanm0000/polars.git
git fetch deanm0000
git checkout datetime_timezone_from_dicts
# make your changes
git commit -a -m 'make awesome changes :sunglasses:'
git push

@deanm0000
Copy link
Collaborator Author

@alexander-beedie I added you as a collaborator but it didn't ask for permissions so not sure if it'll actually do anything. Hopefully (probably) it'll be moot and @MarcoGorelli's suggestion will work.

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Jul 24, 2024

@alexander-beedie I added you as a collaborator but it didn't ask for permissions so not sure if it'll actually do anything.

This did the trick - just pushed successfully. Odd that it was required, but such are the vagaries of git(hub)! Will approve/merge once CI is green ✅

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Jul 24, 2024

> git commit -a -m 'make awesome changes :sunglasses:'

My favourite kind of changes! 😎

@alexander-beedie alexander-beedie merged commit e96dd26 into pola-rs:main Jul 24, 2024
14 checks passed
@deanm0000 deanm0000 deleted the datetime_timezone_from_dicts branch August 26, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix python Related to Python Polars
Projects
None yet
3 participants