Skip to content

Conversation

tlm365
Copy link
Contributor

@tlm365 tlm365 commented Jun 4, 2025

Resolves #143.

Blocked until this PR is merged and released: chronotope/chrono-tz#202

@tlm365 tlm365 requested a review from a team as a code owner June 4, 2025 09:37

This comment was marked as duplicate.

Copy link

cla-bot bot commented Jun 4, 2025

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Tai Le Manh.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@jtcohen6
Copy link
Collaborator

jtcohen6 commented Jun 4, 2025

@tlm365 Thanks for another contribution! Since your first PR, I enabled a CLA check on this repo - could you give this a read and sign it? If you have signed, it might be an issue with how your email is configured for signing git commits

@dbeatty10 dbeatty10 changed the title Make modules.pytz.timezone compatible wih dbt-core Make modules.pytz.timezone compatible with dbt-core Jun 4, 2025
@tlm365
Copy link
Contributor Author

tlm365 commented Jun 5, 2025

@tlm365 Thanks for another contribution! Since your first PR, I enabled a CLA check on this repo - could you give this a read and sign it? If you have signed, it might be an issue with how your email is configured for signing git commits

@jtcohen6 Thanks for reviewing! I've signed it.

Copy link
Contributor

@wolfram-s wolfram-s left a comment

Choose a reason for hiding this comment

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

Great. Thanks for making time zone conversion less strict and thus less annoying.

@TIHan
Copy link
Collaborator

TIHan commented Jun 24, 2025

Changes have been merged internally. Thank you @tlm365 !

@TIHan TIHan closed this Jun 24, 2025
@TIHan TIHan reopened this Jun 25, 2025
@TIHan
Copy link
Collaborator

TIHan commented Jun 25, 2025

@tlm365 - Re-opening.
We had to revert the change unfortunately due to the uncased crate not being referenced correctly in the chrono-tz crate. We were not able to build dbt-fusion successfully on its own. Not your fault, and this is a change that we want.

Are you able to build dbt-fusion with these changes you have proposed? We are also currently working on getting CI working for dbt-fusion.

fa-assistant added a commit that referenced this pull request Jun 26, 2025
* COPYBARA PUBLIC PR: https://github.com/dbt-labs/dbt-fusion/145
Make modules.pytz.timezone compatible with dbt-core

GitOrigin-RevId: 7fda2a1

* Fixing build

* Added changelog. Trying to fix build.

* Reset Cargo.lock and update it again

* Declare chrono-tz case-insensitive feature workspace-wide

chrono-tz & chrono-tz-build makes incorrect assumptions about how cargo
works, leading to compile time failures.
This can happen when something enables chrono-tz-build crate > case-insensitive feature,
without enabling chrono-tz crate > case-insensitive feature.

* Added changelog

* Added changelog

---------

Co-authored-by: Tai Le Manh <[email protected]>
Co-authored-by: TIHan <[email protected]>
Co-authored-by: Piotr Findeisen <[email protected]>
GitOrigin-RevId: 20df6e9ab7ec01ec62a9ff52db45aafc3d22bc33
fa-assistant pushed a commit that referenced this pull request Jun 26, 2025
…encing `uncased` (#4143)

* Reverted #145 as there is an issue with chrono-tz referencing uncased

* Added changelog

* Fix consistency

GitOrigin-RevId: 6e1f6ae9728f12383ae75d2fe7f373e11d3e6710
@tlm365
Copy link
Contributor Author

tlm365 commented Jun 26, 2025

@tlm365 - Re-opening. We had to revert the change unfortunately due to the uncased crate not being referenced correctly in the chrono-tz crate. We were not able to build dbt-fusion successfully on its own. Not your fault, and this is a change that we want.

Are you able to build dbt-fusion with these changes you have proposed? We are also currently working on getting CI working for dbt-fusion.

Oh, my apologies 🙏 I just verified via unit tests and didn’t check the full project build.

I’ve raised a PR to address the issue in chrono-tz, but it seems we’ll need to wait for the next release if we want to keep the current implementation. Otherwise, we may need to explore an alternative approach.

I’ll continue tracking this. Really appreciate you reviewing and report it to me @TIHan!

@TIHan
Copy link
Collaborator

TIHan commented Jun 26, 2025

I’ve raised a PR to address the issue in chrono-tz, but it seems we’ll need to wait for the next release if we want to keep the current implementation. Otherwise, we may need to explore an alternative approach.

Oh, thank you very much for reaching out to them regarding the issue; that helps us a lot.

I’ll continue tracking this. Really appreciate you reviewing and report it to me @TIHan!

Of course! We love the change and we did our best to figure it out internally but it's ultimately up to chrono-tz.

@TIHan
Copy link
Collaborator

TIHan commented Jun 26, 2025

Oh, my apologies 🙏 I just verified via unit tests and didn’t check the full project build.

Also, no worries. It's mainly our fault. We need to have CI running build + tests on the public repo, but it's taking some time to get that set up.

Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

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

@tlm365 chronotope/chrono-tz#202 was closed by the maintainer of chrono-tz.

Do you see any path forward for this PR?

@cla-bot cla-bot bot added the cla:yes label Jul 14, 2025
@tlm365
Copy link
Contributor Author

tlm365 commented Jul 14, 2025

@tlm365 chronotope/chrono-tz#202 was closed by the maintainer of chrono-tz.

Do you see any path forward for this PR?

@dbeatty10 Thanks for reviewing.
Somehow, I have successfully built the project after merging the latest code from main. This PR now works fine and builds successfully on my machine (MacOS-ARM).

@dbeatty10 dbeatty10 self-requested a review July 14, 2025 14:45
@dbeatty10 dbeatty10 dismissed their stale review July 14, 2025 15:14

Dismissing per #145 (comment)

@TIHan
Copy link
Collaborator

TIHan commented Jul 23, 2025

@tlm365 I'm still working through this internally :) - I still had some issues with building - nothing on your end you can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] modules.pytz.timezone() is case-insensitive in dbt Core, but case-sensitive in the dbt Fusion engine
5 participants