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

VTimeZone DAYLIGHT and STANDARD collections #304

Merged
merged 20 commits into from
Nov 14, 2017
Merged

Conversation

beriniwlew
Copy link
Collaborator

@beriniwlew beriniwlew commented Jun 22, 2017

VTimeZone.FromSystemTimeZone method now generates DAYLIGHT or STANDARD collections

@beriniwlew beriniwlew changed the title Added null support for constructors / properties in Organizer.cs, Cal… Null support for constructors / properties in Organizer.cs, Cal… Jun 22, 2017
@beriniwlew beriniwlew changed the title Null support for constructors / properties in Organizer.cs, Cal… VTimeZone.FromSystemTimeZone method does not create DAYLIGHT or STANDARD collections Jul 14, 2017
@beriniwlew beriniwlew changed the title VTimeZone.FromSystemTimeZone method does not create DAYLIGHT or STANDARD collections VTimeZone DAYLIGHT or STANDARD collections Jul 14, 2017
@beriniwlew beriniwlew changed the title VTimeZone DAYLIGHT or STANDARD collections VTimeZone DAYLIGHT and STANDARD collections Jul 14, 2017
@mattwoberts
Copy link

Hi. This looks interesting - it adds support for adding the vtimezone data to the ical feed, is that correct? Is there a reason it's not been reviewed or accepted?

@beriniwlew
Copy link
Collaborator Author

Correct.

I didn't follow some of the guidelines of making a proper pull request... so I'm assuming it's that.

@rianjs
Copy link
Owner

rianjs commented Oct 5, 2017

I haven’t had the time or energy is all.

@beriniwlew
Copy link
Collaborator Author

Ok, whew! At least I'm not being ignored. ;)

@programcsharp
Copy link

Any news on this one, or anything I can do to help out? This is a blocker for us being able to upgrade from DDay.iCal...

@rianjs
Copy link
Owner

rianjs commented Nov 9, 2017

I’ll have a look at it in the next few days. I have some major breaking changes I’m making right now. (I.e., v4 is on the way.)

@rianjs
Copy link
Owner

rianjs commented Nov 13, 2017

I love this change. I'm sorry I didn't get to it sooner. It's implemented exactly how I would have done it (using the NodaTime databases), and even the design and coding style is similar.

I'm not going to merge this PR. I'm going to lift its changes into the version in net-core, whose major version will be bumped to 4 this week. v4 contains many more bugfixes ( #320, #330, #331, #332, #318 ), and thematically is almost entirely time-focused, so this fits with that theme very nicely.

At this point, I have to stop supporting v2.

@pomoika
Copy link

pomoika commented Nov 13, 2017

Is it possible to merge this pull request into v2 as well? Lots of people would be happy to have this functionality in current version of Ical.Net for existing projects that can't use net-core.
Thanks you in advance.

@rianjs
Copy link
Owner

rianjs commented Nov 13, 2017

net-core targets NetStandard 1.3, which means applications .NET 4.6+ can use it.

Your applications are < 4.6?

@beriniwlew
Copy link
Collaborator Author

Awesome! Glad you like it. I look forward to contributing more! -Cheers

@rianjs
Copy link
Owner

rianjs commented Nov 13, 2017

I lifted these changes into net-core, which works with NetStandard 1.3+ (which is .NET 4.6+). They're available in nuget version 4.0.0: https://www.nuget.org/packages/Ical.Net/4.0.0

@pomoika
Copy link

pomoika commented Nov 14, 2017

@rianjs Your applications are < 4.6?

Yes, we have a legacy application that targets .NET 4.0. I would appreciate if you could incorporate this change as is into the final version of Ical.Net v2 nuget package even without future support.
Thanks!

@rianjs rianjs merged commit ede9d5b into rianjs:master Nov 14, 2017
@rianjs
Copy link
Owner

rianjs commented Nov 14, 2017

I suppose. v2 really is deprecated, though.

rianjs pushed a commit that referenced this pull request Nov 14, 2017
@rianjs
Copy link
Owner

rianjs commented Nov 14, 2017

OK, it's 2.3.5 on nuget.

@jocull
Copy link

jocull commented Mar 9, 2018

Just wanted to chime in and say thank you to everyone who worked on this. We were injecting tzdb timezone identifiers into our iCal feeds which was fine on everything we tested (Google Calendar, iCalendar) with but Outlook, which seems to serialize out using Windows system TZ names. I pull in this change and had us fixed up with real TZ info in no time.

We did try using the 2.3.5 for minimal friction, but it seems to have a hard dependency on NodaTime 1.x (and we are using 2.x). I got errors when trying it related to how they rearranged the SystemClock API in 2.x. The jump from .NET 4.5 to 4.6+ was an easy one though in our case.

@rianjs
Copy link
Owner

rianjs commented Mar 9, 2018

We did try using the 2.3.5 for minimal friction, but it seems to have a hard dependency on NodaTime 1.x (and we are using 2.x). I got errors when trying it related to how they rearranged the SystemClock API in 2.x. The jump from .NET 4.5 to 4.6+ was an easy one though in our case.

There are a few differences between NodaTime v1 and v2, not the least of which are different data sources behind the scenes. I actually had to tweak the unit test assertions for the net-core version because they were slightly different for some relatively obscure corner cases/time zones.

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.

6 participants