Add Calendar class #111
Add Calendar class #111
Conversation
|
Just a quick question before I dig deeper into the code review: What benefits does the ICU calendar class give over just using .NET's Calendar class? |
|
The PR fails two tests on Linux in the CI build: |
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @j-troc)
source/icu.net/Calendar/Calendar.cs, line 157 at r1 (raw file):
protected Locale _locale;
nitpick: extra whitespace
source/icu.net/Calendar/Calendar.cs, line 193 at r1 (raw file):
/// <param name="dateTime">The given date in UTC (GMT) time.</param> public void SetTime(DateTime dateTime) {
It might make sense to work with dateTime.ToUniversalTime(), then the date can be in any timezone.
source/icu.net/Calendar/Calendar.cs, line 227 at r1 (raw file):
/// /// The only difference between roll() and add() is that roll() does not change the value of more significant fields /// when it reaches the minimum or maximum of its range, whereas add() does.
It might be helpful to add a unit test that demonstrates the difference between the two methods.
source/icu.net/Calendar/Calendar.cs, line 464 at r1 (raw file):
set { NativeMethods.ucal_setAttribute(_calendarHandle, UCalendarAttribute.FirstDayOfWeek, value);
Shouldn't this be UCalendarAttribute.MinimalDaysInFirstWeek?!
source/icu.net/Calendar/Calendar.cs, line 507 at r1 (raw file):
get { return NativeMethods.ucal_get(_calendarHandle, UCalendarDateFields.Era, out _);
Check error? (same for the other uses of ucal_get)
source/icu.net/Calendar/Calendar.cs, line 646 at r1 (raw file):
/// Gets or seths whether the hour is before or after noon. /// </summary> public int AmPm
Wouldn't it be better to use bool as return value here?
source/icu.net/Calendar/Calendar.cs, line 806 at r1 (raw file):
_calendarHandle.Dispose();
This should go in the if (disposing) block. In the case of disposing being false the GC takes care of calling dispose on the calendar handle.
source/icu.net/Calendar/Calendar.cs, line 807 at r1 (raw file):
_disposingValue = true
There seems to be a mismatch between the name and intention of the variable and what it does. If you want to detect redundant calls then you'd have to set it to true right after if (!_disposingValue) and to false before leaving the block. The way it is currently used checks whether or not the object is already disposed - a better name name would then be _isDisposed.
source/icu.net/Calendar/GregorianCalendar.cs, line 40 at r1 (raw file):
{ var handle = NativeMethods.ucal_clone(_calendarHandle, out ErrorCode status); return new GregorianCalendar(handle);
We should probably copy _locale as well.
Error checking?
source/icu.net/Calendar/GregorianCalendar.cs, line 45 at r1 (raw file):
public override bool InDaylightTime() { return NativeMethods.ucal_inDaylightTime(_calendarHandle, out _);
We should probably do some error checking, otherwise returning false can mean not in daylight savings time AND something went wrong which makes it hard to debug in a client app.
source/icu.net/NativeMethods/NativeMethods_Calendar.cs, line 76 at r1 (raw file):
[UnmanagedFunctionPointer(CallingConvention.Cdecl, CharSet = CharSet.Unicode)] internal delegate int ucal_getAttributeDelegate(
Strange that you have it with three parameters here - the docs list only two. Are you sure this is correct?
If this is correct a unit test would be good that proves that this is the correct/working signature (and even if this wrong a unit test would be nice...)
source/icu.net/NativeMethods/NativeMethods_Calendar.cs, line 185 at r1 (raw file):
out string result,
I'd prefer if you change this to IntPtr result and deal with converting to a string in the wrapper method. See for example Normalizer2.GetDecomposition and unorm2_getDecomposition in NativeMethods_Normalize.cs.
This would be more in alignment of the way it's done elsewhere in icu.net.
source/icu.net/NativeMethods/NativeMethods_Calendar.cs, line 237 at r1 (raw file):
} public static void ucal_roll( Calendar.SafeCalendarHandle cal,
nitpick: indentation
source/icu.net/NativeMethods/NativeMethods_Calendar.cs, line 322 at r1 (raw file):
out string result
I'd prefer if you change this to IntPtr result and deal with converting to a string in the wrapper method. See for example Normalizer2.GetDecomposition and unorm2_getDecomposition in NativeMethods_Normalize.cs.
This would be more in alignment of the way it's done elsewhere in icu.net.
source/icu.net.tests/CalendarTests.cs, line 18 at r1 (raw file):
{ var timezone = new TimeZone("AST"); var cal = new GregorianCalendar(timezone);
Since Calendar implements IDisposable, all cals have to be wrapped in a using statement:
using (var cal = new GregorianCalendar(timezone))
{
....
}
Or make cal a field and dispose it in a TearDown method after each test.
source/icu.net.tests/CalendarTests.cs, line 42 at r1 (raw file):
{ var cal = new GregorianCalendar(); cal.Month = Calendar.UCalendarMonths.September;
For documentation purposes you might want to set cal.DayOfMonth = 4; and check that below.
source/icu.net.tests/CalendarTests.cs, line 58 at r1 (raw file):
cal2.DayOfMonth = 10; Assert.AreEqual(5, cal1.DayOfMonth);
Add Assert.AreEqual(10, cal2.DayOfMonth);
source/icu.net.tests/CalendarTests.cs, line 230 at r1 (raw file):
cal.Minute = 0; cal.Month = Calendar.UCalendarMonths.March; cal.DayOfMonth = 13;
Things would be easier to read if you'd sort the properties according to time: Year / Month / DayOfMonth / HourOfDay / Minute. Same for other tests.
source/icu.net.tests/CalendarTests.cs, line 358 at r1 (raw file):
[Test]
nitpick: extra empty line
source/icu.net.tests/CalendarTests.cs, line 374 at r1 (raw file):
[Test] public void GetTimeZoneTest()
That should be public void GetTimeZoneInfoTest()
|
A team member has to approve this pull request on the CI server before it can be built... |
There was a problem hiding this comment.
Reviewed 1 of 4 files at r2.
Reviewable status: 1 of 4 files reviewed, 15 unresolved discussions (waiting on @ermshiperete and @j-troc)
source/icu.net.tests/CalendarTests.cs, line 275 at r2 (raw file):
var val1 = cal.FirstDayOfWeek; Assert.AreEqual(Calendar.UCalendarDaysOfWeek.Sunday, val0);
Please write this as separate tests. You can use the TestCase attribute for that. See below.
source/icu.net.tests/CalendarTests.cs, line 294 at r2 (raw file):
Assert.AreEqual(2, val0);
Please write this as separate tests. You can use the TestCase attribute for that. See below.
source/icu.net.tests/CalendarTests.cs, line 312 at r2 (raw file):
var val1 = cal.WeekOfYear; Assert.AreEqual(2, val0);
Please write this as separate tests. You can use the TestCase attribute for that. See below.
source/icu.net.tests/CalendarTests.cs, line 395 at r2 (raw file):
var val1 = cal.WeekOfMonth; Assert.AreEqual(2, val0);
Please write this as separate tests. You can use the TestCase attribute for that. See below.
source/icu.net.tests/CalendarTests.cs, line 409 at r2 (raw file):
var era0 = cal.Era; Assert.AreEqual(1, era1);
Please write this as separate tests. You can use the TestCase attribute for that. See below.
source/icu.net.tests/CalendarTests.cs, line 445 at r2 (raw file):
var offset1 = cal.DstOffset; Assert.AreEqual(expected0, offset0);
Please write this as separate tests. You can use the TestCase attribute for that:
[TestCase(Calendar.UCalendarMonths.July, ExpectedResult = 3600000 /* 60min * 60s * 1000ms */)]
[TestCase(Calendar.UCalendarMonths.January, ExpectedResult = 0)]
public int DstOffsetTest(Calendar.UCalendarMonths month)
{
var zone = new TimeZone("Europe/Paris");
using (var cal = new GregorianCalendar(zone))
{
cal.Month = month;
cal.DayOfMonth = 20;
return cal.DstOffset;
}
}source/icu.net.tests/CalendarTests.cs, line 460 at r2 (raw file):
var val1 = cal.AmPm; Assert.AreEqual(Calendar.UCalendarAMPMs.Am, val0);
Please write this as separate tests. You can use the TestCase attribute for that:
[TestCase( 3, ExpectedResult = Calendar.UCalendarAMPMs.Am)]
[TestCase(14, ExpectedResult = Calendar.UCalendarAMPMs.Pm)]
public Calendar.UCalendarAMPMs AmPmTest(int hourOfDay)
{
using (var cal = new GregorianCalendar(new TimeZone("UTC")))
{
cal.HourOfDay = hourOfDay;
return cal.AmPm;
}
}source/icu.net.tests/CalendarTests.cs, line 474 at r2 (raw file):
var timezone = timezones.FirstOrDefault(tzi => tzi.Id == tzdbId); if(timezone==null)
Please create two separate tests, one for Windows and the other for Linux. You can use [Platform(Exclude="win")] and [Platform(Include="win")] to have them run only on one platform.
source/icu.net.tests/CalendarTests.cs, line 501 at r2 (raw file):
var result = cal.GetTimeZoneInfo(); Assert.IsTrue(result.Id == winId || result.Id == tzdbId);
Please create two separate tests, one for Windows and the other for Linux. You can use [Platform(Exclude="win")] and [Platform(Include="win")] to have them run only on one platform.
|
A team member has to approve this pull request on the CI server before it can be built... |
|
A team member has to approve this pull request on the CI server before it can be built... |
There was a problem hiding this comment.
Reviewed 2 of 4 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @j-troc)
source/icu.net.tests/CalendarTests.cs, line 473 at r3 (raw file):
[Test] [Platform(Include = "win")] public void GetTilmeZoneInfoTestWin()
Typo: should be GetTimeZoneInfoTestWin
source/icu.net.tests/CalendarTests.cs, line 481 at r3 (raw file):
var result = cal.GetTimeZoneInfo(); Assert.IsTrue(result.Id == "Central European Standard Time");
Would be better written as Assert.AreEqual("Central European Standard Time", result.Id) or Assert.That(result.Id, Is.EqualTo("Central European Standard Time"))
source/icu.net.tests/CalendarTests.cs, line 497 at r3 (raw file):
var result = cal.GetTimeZoneInfo(); Assert.IsTrue(result.Id == tzdbId);
See above
|
A team member has to approve this pull request on the CI server before it can be built... |
|
It builds just fine on my machines. |
Did you try on Linux? From the build logs it looks like it gets some memory corruption - possibly the signature for an unmanaged method isn't quite correct yet. (Note that |
|
A team member has to approve this pull request on the CI server before it can be built... |
1 similar comment
|
A team member has to approve this pull request on the CI server before it can be built... |
ermshiperete
force-pushed
the
calendar
branch
from
77e696d
to
691f58e
Compare
This change is