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

consolidate pendulum use in prefect._internal #17072

Merged
merged 4 commits into from
Feb 11, 2025
Merged

Conversation

zzstoatzz
Copy link
Collaborator

@zzstoatzz zzstoatzz commented Feb 10, 2025

working on #16910

@zzstoatzz zzstoatzz changed the title rm weird pendulum compat code consolidate pendulum use in prefect._internal Feb 10, 2025
@zzstoatzz zzstoatzz force-pushed the pendulum-consolidation-5 branch from df314a9 to 219fa7a Compare February 10, 2025 17:24
Copy link

codspeed-hq bot commented Feb 10, 2025

CodSpeed Performance Report

Merging #17072 will not alter performance

Comparing pendulum-consolidation-5 (516eaae) with main (ffd0d50)

Summary

✅ 2 untouched benchmarks

@zzstoatzz zzstoatzz force-pushed the pendulum-consolidation-5 branch 2 times, most recently from 2e3ad8a to 0047e73 Compare February 10, 2025 19:00
@zzstoatzz zzstoatzz marked this pull request as ready for review February 10, 2025 20:31
@zzstoatzz zzstoatzz force-pushed the pendulum-consolidation-5 branch from 0047e73 to fc84ce1 Compare February 11, 2025 16:09
Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

One small comment about naming, but otherwise LGTM!

return pendulum.tz.timezones()


def pendulum_instance(v: DateTime) -> DateTime:
Copy link
Member

Choose a reason for hiding this comment

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

Could we name this something like create_datetime_instance? Also, why do we need to call .instance twice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice calls on both counts, I can just use the outside .instance

@zzstoatzz zzstoatzz merged commit 3c5112d into main Feb 11, 2025
46 checks passed
@zzstoatzz zzstoatzz deleted the pendulum-consolidation-5 branch February 11, 2025 16:46
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