Add TimeZone class #108

j-troc · 2019-01-14T14:37:34Z

This change is

j-troc · 2019-01-17T11:05:12Z

Could you share test failure details? Link is inaccessible.

ermshiperete · 2019-01-17T14:00:38Z

Thanks for your contribution!

Sorry, I had intended to share the failed test, but didn't get around yet. Anyway, here it is:

Error Message

  Expected: 632
  But was:  621

Stacktrace

  at Icu.Tests.TimeZoneTests.GetTimeZonesTest () [0x00016] in <0ce791bf19c64c649fd393bc30a4276c>:0

That's running the tests on Ubuntu 18.04; the tests succeed on Windows. I haven't looked yet if this is caused by Linux/Windows difference, or if it is caused by using a different ICU version.

j-troc · 2019-01-18T14:27:12Z

I changed the test because number of timezones varies depending on ICU version.

wiertel

Reviewed 1 of 1 files at r2.
Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @j-troc)

source/icu.net/Timezone/Timezone.cs, line 15 at r2 (raw file):

		public string ID => zoneID;

		public TimeZone(string id)

missing documentation

source/icu.net/Timezone/Timezone.cs, line 37 at r2 (raw file):

			{
				string str = en.Next();
				while (str != null)

this List building from SafeEnumeratorHandle is used couple times, is there a utility method doing exactly that, maybe put this code into private method?

source/icu.net/Timezone/Timezone.cs, line 51 at r2 (raw file):

		/// <summary>
		/// Returns an enumeration over system TimeZones with the given filter conditions.

space at the end of the line

source/icu.net/Timezone/USystemTimeZoneType.cs, line 4 at r2 (raw file):

using System.Collections.Generic;
using System.Linq;
using System.Text;

none of these namespaces is used

wiertel

Reviewed 1 of 4 files at r1.
Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @j-troc)

wiertel

Reviewed 2 of 4 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @j-troc)

j-troc · 2019-02-01T11:49:42Z

These changes build on my machine. What is the build error?

ermshiperete · 2019-02-15T07:42:01Z

Sorry, I was travelling. There was a hick-up on the server. I restarted the build and now it passed.

j-troc · 2019-03-13T09:37:43Z

Does the code need changes?

ermshiperete

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @j-troc)

source/icu.net/Timezone/Timezone.cs, line 14 at r3 (raw file):

		/// Returns TimeZone's ID.
		/// </summary>
		public string ID => zoneID;

I think I'd prefer to rename this to Id (lowercase d)

source/icu.net/Timezone/Timezone.cs, line 17 at r3 (raw file):

ID

nitpick: replace ID with <paramref name="id"/>

source/icu.net/Timezone/Timezone.cs, line 19 at r3 (raw file):

"id">IDthe

nitpick: Remove "ID" and tab character after it.

source/icu.net/Timezone/Timezone.cs, line 79 at r3 (raw file):

		}

		private static List<TimeZone> CreateTimeZoneList(Func<Tuple<SafeEnumeratorHandle, ErrorCode>> enumeratorSource)

This method could return IEnumerable<TimeZone> as well.

source/icu.net/Timezone/Timezone.cs, line 83 at r3 (raw file):

			List<TimeZone> timeZones = new List<TimeZone>();

			(SafeEnumeratorHandle en, ErrorCode ec) = enumeratorSource();

Please rename the variable names to something a bit less cryptic, e.g. enumerator and errorCode.

source/icu.net/Timezone/Timezone.cs, line 87 at r3 (raw file):

			try
			{
				string str = en.Next();

Rename to something less cryptic, e.g. timezoneId

source/icu.net/Timezone/Timezone.cs, line 109 at r3 (raw file):

			var timeZoneName = NativeMethods.GetUnicodeString((ptr, length) =>
			{
				length = NativeMethods.ucal_getDefaultTimeZone(ptr, length, out ErrorCode err);

Shouldn't we check the error code? I.e. add the line ExceptionFromErrorCode.ThrowIfError(ec) ?

source/icu.net/Timezone/Timezone.cs, line 159 at r3 (raw file):

			return NativeMethods.GetUnicodeString((ptr, length) =>
			{
				length = NativeMethods.ucal_getWindowsTimeZoneId(id, ptr, length, out ErrorCode err);

Shouldn't we check the error code? I.e. add the line ExceptionFromErrorCode.ThrowIfError(ec) ?

source/icu.net/Timezone/Timezone.cs, line 176 at r3 (raw file):

			return NativeMethods.GetUnicodeString((ptr, length) =>
			{
				length = NativeMethods.ucal_getTimeZoneIDForWindowsId(winId, region, ptr, length, out ErrorCode err);

Shouldn't we check the error code? I.e. add the line ExceptionFromErrorCode.ThrowIfError(ec) ?

source/icu.net.tests/TimeZoneTests.cs, line 71 at r3 (raw file):

			Assert.GreaterOrEqual(timezones.Count(), 4);
			Assert.AreEqual(1, timezones.Count(tz => tz.ID == "Atlantic/Azores"));
			Assert.AreEqual(1, timezones.Count(tz => tz.ID == "America/Scoresbysund"));

It might make sense to check the other two zones as well.

source/icu.net.tests/TimeZoneTests.cs, line 78 at r3 (raw file):

		{
			var expected = new TimeZone("AST");
			TimeZone.SetDefault(expected);

This test is identical to DefaultTimeoneTest() above. You could change it to use the .NET TimeZoneInfo class for getting the expected timezone:

var expected = TimeZoneInfo.Local;

ermshiperete

Thanks for the reminder. A few small suggestions.

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @j-troc)

j-troc

Reviewable status: 2 of 4 files reviewed, 11 unresolved discussions (waiting on @ermshiperete and @wiertel)

source/icu.net/Timezone/Timezone.cs, line 15 at r2 (raw file):

Previously, wiertel (Piotr Szydełko) wrote…

missing documentation

Done.

source/icu.net/Timezone/Timezone.cs, line 37 at r2 (raw file):

Previously, wiertel (Piotr Szydełko) wrote…

this List building from SafeEnumeratorHandle is used couple times, is there a utility method doing exactly that, maybe put this code into private method?

Done.

source/icu.net/Timezone/Timezone.cs, line 51 at r2 (raw file):

Previously, wiertel (Piotr Szydełko) wrote…

space at the end of the line

Done.

source/icu.net/Timezone/Timezone.cs, line 14 at r3 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

I think I'd prefer to rename this to Id (lowercase d)

Done.

source/icu.net/Timezone/Timezone.cs, line 79 at r3 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

This method could return IEnumerable<TimeZone> as well.

Done.

source/icu.net/Timezone/Timezone.cs, line 109 at r3 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

Shouldn't we check the error code? I.e. add the line ExceptionFromErrorCode.ThrowIfError(ec) ?

Errors are chcecked inside GetUnicodeString method so there's no need to duplicate that.

source/icu.net/Timezone/Timezone.cs, line 159 at r3 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

Shouldn't we check the error code? I.e. add the line ExceptionFromErrorCode.ThrowIfError(ec) ?

Errors are chcecked inside GetUnicodeString method so there's no need to duplicate that.

source/icu.net/Timezone/Timezone.cs, line 176 at r3 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

Shouldn't we check the error code? I.e. add the line ExceptionFromErrorCode.ThrowIfError(ec) ?

Errors are chcecked inside GetUnicodeString method so there's no need to duplicate that.

source/icu.net/Timezone/USystemTimeZoneType.cs, line 4 at r2 (raw file):

Previously, wiertel (Piotr Szydełko) wrote…

none of these namespaces is used

Done.

source/icu.net.tests/TimeZoneTests.cs, line 71 at r3 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

It might make sense to check the other two zones as well.

Done.

source/icu.net.tests/TimeZoneTests.cs, line 78 at r3 (raw file):

Previously, ermshiperete (Eberhard Beilharz) wrote…

This test is identical to DefaultTimeoneTest() above. You could change it to use the .NET TimeZoneInfo class for getting the expected timezone:
var expected = TimeZoneInfo.Local;

Value returned by TimeZoneInfo.Local will be different based on

ermshiperete

Reviewed 2 of 2 files at r4.
Dismissed @wiertel from 4 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved

ermshiperete

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

ermshiperete

Reviewable status: complete! all files reviewed, all discussions resolved

ermshiperete · 2019-03-15T17:04:58Z

Thanks @j-troc !

Add TimeZone class

7048c96

Fix time zone tests

202f074

Add TZ version method

fa5f1d9

wiertel reviewed Jan 31, 2019

View reviewed changes

Fix review issues

e822ed5

ermshiperete requested changes Mar 14, 2019

View reviewed changes

ermshiperete reviewed Mar 14, 2019

View reviewed changes

Fix review issues

0ed2029

j-troc commented Mar 15, 2019

View reviewed changes

ermshiperete approved these changes Mar 15, 2019

View reviewed changes

Fix DefaultTimezoneTests

a8ab036

ermshiperete approved these changes Mar 15, 2019

View reviewed changes

ermshiperete merged commit 2e74ff2 into sillsdev:master Mar 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TimeZone class #108

Add TimeZone class #108

j-troc commented Jan 14, 2019 •

edited by ermshiperete

Loading

j-troc commented Jan 17, 2019

ermshiperete commented Jan 17, 2019

j-troc commented Jan 18, 2019 •

edited

Loading

wiertel left a comment

wiertel left a comment

wiertel left a comment

j-troc commented Feb 1, 2019

ermshiperete commented Feb 15, 2019

j-troc commented Mar 13, 2019

ermshiperete left a comment

ermshiperete left a comment

j-troc left a comment

ermshiperete left a comment

ermshiperete left a comment

ermshiperete left a comment

ermshiperete commented Mar 15, 2019

Add TimeZone class #108

Add TimeZone class #108

Conversation

j-troc commented Jan 14, 2019 • edited by ermshiperete Loading

j-troc commented Jan 17, 2019

ermshiperete commented Jan 17, 2019

j-troc commented Jan 18, 2019 • edited Loading

wiertel left a comment

Choose a reason for hiding this comment

wiertel left a comment

Choose a reason for hiding this comment

wiertel left a comment

Choose a reason for hiding this comment

j-troc commented Feb 1, 2019

ermshiperete commented Feb 15, 2019

j-troc commented Mar 13, 2019

ermshiperete left a comment

Choose a reason for hiding this comment

ermshiperete left a comment

Choose a reason for hiding this comment

j-troc left a comment

Choose a reason for hiding this comment

ermshiperete left a comment

Choose a reason for hiding this comment

ermshiperete left a comment

Choose a reason for hiding this comment

ermshiperete left a comment

Choose a reason for hiding this comment

ermshiperete commented Mar 15, 2019

j-troc commented Jan 14, 2019 •

edited by ermshiperete

Loading

j-troc commented Jan 18, 2019 •

edited

Loading