Skip to content

Conversation

sergioisidoro
Copy link

@sergioisidoro sergioisidoro commented Jun 17, 2025

Trying to approach #568

May also fix:
#535

@wwahammy
Copy link

wwahammy commented Aug 26, 2025

@sergioisidoro this is great work! I could really use this feature, do you need any help on getting this PR across the finish line?

@sergioisidoro
Copy link
Author

sergioisidoro commented Aug 26, 2025

@wwahammy I'm fairly pessimistic about seeing this merged in this project. This is a breaking change, and the last commit to main has been done last year. There have been other people struggling with this issue with some atempts at solving it, and they have not been merged.

At the moment I'm using my fork installing the gem through git, and I've been considering publishing my fork to rubygems for our own use, as we're starting to use recurrent events more and more, with iCal as serialization format with the API.

But if you find any way of helping this through the finnish line, I would appreciate it. Or if we would find enough people to help with the maintenace of a separate fork?

@wwahammy
Copy link

But if you find any way of helping this through the finnish line, I would appreciate it. Or if we would find enough people to help with the maintenace of a separate fork?

I don't think I have any more influence than you on the project. I'd be open to helping with the fork.

@wwahammy
Copy link

wwahammy commented Aug 27, 2025

Oh, one thing I had been meaning to ask: is the format you use for the timezones follow the iCal standard? I haven't looked into it so I wasn't sure but I wanted to know.

On particular use-case is that, if we can serialize to standard iCal, we could use a postgres extensions supporting iCal to do actual queries based upon the values.

@sergioisidoro
Copy link
Author

is the format you use for the timezones follow the iCal standard?

Yes, and no. The standard does not specify a specific time zone format

Note: The specification of a global time zone registry is not
addressed by this document and is left for future study.
However, implementers may find the Olson time zone database [TZ]
a useful reference. It is an informal, public-domain collection
of time zone information, which is currently being maintained by
volunteer Internet participants, and is used in several
operating systems. This database contains current and historical
time zone information for a wide variety of locations around the
globe; it provides a time zone identifier for every unique time
zone rule set in actual use since 1970, with historical data
going back to the introduction of standard time.

https://www.ietf.org/rfc/rfc2445.txt

However, it suggests the Olson tz database - ie the timezone database matinained by IANA. The current example of the timezone identifier can be seen here: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones

And as you can see there are same abbreviations for multiple timezones (the old implementation).
This implementation works with at least the javascript library implementations of the iCal standard.

From postgres docs I can see that the "full timezome" format is supported as input

Screenshot 2025-08-28 at 11 17 39

@pacso
Copy link
Collaborator

pacso commented Aug 29, 2025

Hello,

Thank you for your work on this! Am keen to keep this project alive. I will try to put some time aside this weekend to review this PR properly and see if there's a way we can get this merged.

@wwahammy
Copy link

Hello,

Thank you for your work on this! Am keen to keep this project alive. I will try to put some time aside this weekend to review this PR properly and see if there's a way we can get this merged.

Thank you for all of the work you do @pacso! Totally get that you're doing this on the side and that's a lot of work. If you're open to it, it might make sense to work together on developing a path forward that spreads the workload more so you're not responsible for so much. We're all benefiting so we should all pitch-in after all.

Copy link
Author

@sergioisidoro sergioisidoro left a comment

Choose a reason for hiding this comment

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

I self reviewed the PR :) there were a few typos and a small nitpick. This work was done a bit in a hurry to fix our bugs with the lost time in serialization.

Let me know if there's anything else that needs attention on this PR, and I'll fixe everything in one go

time = time.dup.utc if force_utc

# Keep timezone. strftime will serializer short versions of time zone (eg. EEST),
# which are not reversivible, as there are many repeated abbreviated zones. This will result in
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
# which are not reversivible, as there are many repeated abbreviated zones. This will result in
# which are not reversible, as there are many repeated abbreviated zones. This will result in

Comment on lines +50 to +51
# Convert to UTC as TZID=+xxxx format is not recognized by JS libraries
warn "IceCube: Time object does not have timezone info. Assuming UTC: #{caller(1..1).first}"
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
# Convert to UTC as TZID=+xxxx format is not recognized by JS libraries
warn "IceCube: Time object does not have timezone info. Assuming UTC: #{caller(1..1).first}"
# Convert to UTC as TZID=+xxxx format is not recognized by JS libraries
warn "IceCube: Time object does not have timezone info. Coercing into UTC: #{caller(1..1).first}"

Schedule.from_hash data
end

def self.deserialize_time_with_tzid(time_value, tzid)
Copy link
Author

Choose a reason for hiding this comment

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

This should maybe be moved into the TimeUtil class since it's a wrapper for the TimeUtil.deserialize_time

Comment on lines +44 to +47
rescue ArgumentError
# If the timezone is invalid, fall back to standard deserialization
# Perhaps we want to log this?
TimeUtil.deserialize_time(time_value)
Copy link
Author

Choose a reason for hiding this comment

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

Should we issue a warning here as well?

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