+semver:major Made ParseDateTimePermissivelyWithException into an extension method by tombogle · Pull Request #1422 · sillsdev/libpalaso

tombogle · 2025-04-15T18:29:31Z

This is kind of related to SP-2356, though it's not yet clear whether any actual logic changes will be needed in these date-handling methods.

Added test cases for ToISO8601TimeFormatWithUTCString and some unit tests for Made ParseDateTimePermissivelyWithException

This change is

…ension method Added test cases for ToISO8601TimeFormatWithUTCString and some unit tests for Made ParseDateTimePermissivelyWithException

github-actions · 2025-04-15T18:43:33Z

Palaso Tests

4 files ± 0 4 suites ±0 10m 44s ⏱️ - 9m 11s
4 973 tests + 38 4 743 ✅ + 37 230 💤 +1 0 ❌ ±0
16 171 runs +114 15 479 ✅ +109 692 💤 +5 0 ❌ ±0

Results for commit 2ec4327. ± Comparison against base commit 73469af.

This pull request removes 4 and adds 42 tests. Note that renamed tests count towards both.

SIL.Tests.IO.FileLocationUtilitiesTests ‑ LocateExecutable_NonexistingFile
SIL.Tests.IO.FileLocationUtilitiesTests ‑ LocateExecutable_NonexistingFileThrows
SIL.Tests.IO.FileLocationUtilitiesTests ‑ LocateInProgramFiles_DeepSearch_FindsFileInSubdir
SIL.Tests.IO.FileLocationUtilitiesTests ‑ LocateInProgramFiles_SendInValidSubFolder_DoesNotThrow

SIL.Tests.Extensions.DateTimeExtensionTests ‑ ParseDateTimePermissivelyWithException_NearFutureDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("d-M-yyyy")
SIL.Tests.Extensions.DateTimeExtensionTests ‑ ParseDateTimePermissivelyWithException_NearFutureDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("d/M/yyyy")
SIL.Tests.Extensions.DateTimeExtensionTests ‑ ParseDateTimePermissivelyWithException_NearFutureDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("dd MMM yyyy")
SIL.Tests.Extensions.DateTimeExtensionTests ‑ ParseDateTimePermissivelyWithException_NearFutureDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("dd/MM/yyyy")
SIL.Tests.Extensions.DateTimeExtensionTests ‑ ParseDateTimePermissivelyWithException_NearFutureUSDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("D")
SIL.Tests.Extensions.DateTimeExtensionTests ‑ ParseDateTimePermissivelyWithException_NearFutureUSDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("M-d-yyyy")
SIL.Tests.Extensions.DateTimeExtensionTests ‑ ParseDateTimePermissivelyWithException_NearFutureUSDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("M/d/yyyy")
SIL.Tests.Extensions.DateTimeExtensionTests ‑ ParseDateTimePermissivelyWithException_NearFutureUSDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("MM-dd-yyyy")
SIL.Tests.Extensions.DateTimeExtensionTests ‑ ParseDateTimePermissivelyWithException_NearFutureUSDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("MM/dd/yyyy")
SIL.Tests.Extensions.DateTimeExtensionTests ‑ ParseDateTimePermissivelyWithException_NearFutureUSDatesWithThaiBuddhistCalendar_ReturnsDateWithPastYear("d")
…

This pull request removes 1 skipped test and adds 1 skipped test. Note that renamed tests count towards both.

SIL.Tests.IO.FileLocationUtilitiesTests ‑ LocateInProgramFiles_DeepSearch_FindsFileInSubdir

SIL.Tests.IO.FileLocationUtilitiesTests ‑ LocateInProgramFiles_DeepSearch_FindsFileInSubDir

This pull request skips 1 test.

SIL.Tests.IO.FileLocationUtilitiesTests ‑ LocateInProgramFiles_SendValidProgramDeepSearch_ReturnsProgramPath

♻️ This comment has been updated with latest results.

imnasnainaec

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tombogle)

…teTimePermissivelyWithException to indicate an reasonable range of dates and changed behavior of the method to try to interpret the date according to either the Gregorian calendar or the Buddhist calender in order to get the date to fall within the reasonably expected range. Added more test cases

imnasnainaec

Reviewed all commit messages.
Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @tombogle)

SIL.Core/Extensions/DateTimeExtensions.cs line 106 at r2 (raw file):

		/// </summary>
		public static DateTime ParseDateTimePermissivelyWithException(this string when,
			int oldestReasonablyExpectedYear = 1900, int numberOfDaysIntoFutureReasonablyExpected = 1)

numberOfDaysIntoFutureReasonablyExpected = 1 assumes that somebody isn't commonly using this date-time parsing in event/calendar planning settings. In case somebody is, then having the unit be days instead of years is awkward. Would something like numberOfYearsIntoFutureReasonablyExpected = 10 not work well with the new implementation?

CHANGELOG.md line 70 at r2 (raw file):

- [SIL.Core] Improved efficiency in XmlUtils methods: MakeSafeXml and ConvertMultiParagraphToSafeXml (name corrected), and MakeSafeXmlAttribute.
- [SIL.Core] BREAKING CHANGE (potentially): Made ParseDateTimePermissivelyWithException into an extension method (on string).
- [SIL.Core] BREAKING CHANGE (potentially): Added optional parameters to DateTimeExtensions.ParseDateTimePermissivelyWithException: `oldestReasonablyExpectedYear` and `numberOfDaysIntoFutureReasonablyExpected`. Also, changed behavior of this method to try to interpret the date according to either the Gregorian calendar or the Buddhist calender in order to get the date to fall within the reasonably expected range. This means that depending on the current culture, dates might be interpreted differently from before.

The CHANGELOG entry should probably mention what the default values are for new optional parameters.

SIL.Core.Tests/IO/FileLocationUtilitiesTests.cs line 114 at r2 (raw file):

		[Test]
		public void LocateInProgramFiles_SendInValidSubFolder_DoesNotThrow()

InValid -> Invalid

SIL.Core.Tests/IO/FileLocationUtilitiesTests.cs line 236 at r2 (raw file):

		[Test]
		public void LocateExecutable_NonexistentFileFileThrows()

FileFileThrows -> FileThrows

TestApps/CommandLineRunnerTestApp/CommandLineRunnerTestApp.csproj line 10 at r2 (raw file):

    <AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
    <TargetFrameworks>$(TargetFrameworks);net8.0</TargetFrameworks>
    <GenerateRuntimeConfigurationFiles>true</GenerateRuntimeConfigurationFiles>

Why is this the only test app that needs GenerateRuntimeConfigurationFiles?

.github/workflows/build.yml line 82 at r2 (raw file):

      id: test
      run: dotnet test "$TEST_PATH"/SIL*Tests.dll --filter "(TestCategory != SkipOnTeamCity) & (TestCategory != RequiresAudioInputDevice)" --blame-hang-timeout 5m --logger:"trx;LogFilePrefix=results" --results-directory ./test-results

whitespace on this empty line

…tion and added three alternative versions whose names more clearly indicate their behavior. Minor code cleanup in response to code review.

tombogle

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec and @tombogle)

CHANGELOG.md line 70 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

The CHANGELOG entry should probably mention what the default values are for new optional parameters.

Done

.github/workflows/build.yml line 82 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

whitespace on this empty line

Fixed

SIL.Core/Extensions/DateTimeExtensions.cs line 106 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

numberOfDaysIntoFutureReasonablyExpected = 1 assumes that somebody isn't commonly using this date-time parsing in event/calendar planning settings. In case somebody is, then having the unit be days instead of years is awkward. Would something like numberOfYearsIntoFutureReasonablyExpected = 10 not work well with the new implementation?

In both places I know of where this is currently being used, it is for dates that are typically in the past but usually for relatively recent (modern times) event. Hence the defaults. The reason I wanted to go 1 day into the future was to avoid weird off-by-one issues with time zones. I agree that it looks weirdly inconsistent to use a year for the past and a day offset for the future. I considered the alternative of using an actual DateTime for both, but a) it is impossible to use DateTimes as default values, and b) it felt onerous to require the existing implementations to calculate and supply useful dates. I have decided to deprecate the original version and supply three alternative versions whose names more clearly indicate their behavior.

SIL.Core.Tests/IO/FileLocationUtilitiesTests.cs line 114 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

InValid -> Invalid

Fixed

SIL.Core.Tests/IO/FileLocationUtilitiesTests.cs line 236 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

FileFileThrows -> FileThrows

Fixed

TestApps/CommandLineRunnerTestApp/CommandLineRunnerTestApp.csproj line 10 at r2 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Why is this the only test app that needs GenerateRuntimeConfigurationFiles?

It gets run as part of the unit tests. It was failing, and ChatGPT said that adding that would solve the problem (and it did).

imnasnainaec

Reviewed 1 of 6 files at r2, 1 of 5 files at r3, all commit messages.
Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @tombogle)

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 192 at r3 (raw file):

		/// <param name="expectedResultFormattedAsBuddhistDate"></param>
		[TestCase("9/3/2025 0:00:00", true, "9/3/2568")]
		[TestCase("2-2-1920", true, "2/2/2463")] //

stray // at the end of this line

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 234 at r3 (raw file):

		/// <summary>
		/// For these test cases, well will supply a future date (beyond reasonableMax) and pass

"well will" -> "we will" here and in several other <summary>s

CHANGELOG.md line 32 at r3 (raw file):

- [SIL.Core] Added correctly spelled XmlUtils.ConvertMultiParagraphToSafeXml.
- [SIL.Core] Added DateTimeExtensions.ParseModernPastDateTimePermissivelyWithException (as the most probable replacement for the deprecated version of ParseDateTimePermissivelyWithException).
- [SIL.Core] Added DateTimeExtensions.ParsePastDateTimePermissivelyWithException as a convenience method.

"convenience method" ParsePastDateTimePermissivelyWithException feels like it may be overkill. Unless you have planned use for it, I think it's just adding to maintenance burden.

SIL.Core.Tests/IO/FileLocationUtilitiesTests.cs line 92 at r3 (raw file):

		}

		[Test] // On CI build (GHA) this can time out

// On CI build (GHA) this can time out shows up twice on this test and only once in the test above

tombogle

Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @imnasnainaec)

CHANGELOG.md line 32 at r3 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

"convenience method" ParsePastDateTimePermissivelyWithException feels like it may be overkill. Unless you have planned use for it, I think it's just adding to maintenance burden.

Maybe, but I felt like its presence might help to shed some light on the other options. I'm not 100% sure how/why the Combine uses this, and I wanted to try to provide a possibly easier way forward in case their situation is somewhat different from SayMore.

tombogle

Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @imnasnainaec and @tombogle)

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 192 at r3 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

stray // at the end of this line

Fixed

SIL.Core.Tests/Extensions/DateTimeExtensionTests.cs line 234 at r3 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

"well will" -> "we will" here and in several other <summary>s

Fixed

SIL.Core.Tests/IO/FileLocationUtilitiesTests.cs line 92 at r3 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

// On CI build (GHA) this can time out shows up twice on this test and only once in the test above

Fixed

+semver:major Made ParseDateTimePermissivelyWithException into an ext…

5fe0127

…ension method Added test cases for ToISO8601TimeFormatWithUTCString and some unit tests for Made ParseDateTimePermissivelyWithException

tombogle added wip - don't merge yet api change low priority labels Apr 15, 2025

tombogle self-assigned this Apr 15, 2025

tombogle changed the title ~~+semver:major Made ParseDateTimePermissivelyWithException into an ex…~~ +semver:major Made ParseDateTimePermissivelyWithException into an exteension method Apr 15, 2025

tombogle changed the title ~~+semver:major Made ParseDateTimePermissivelyWithException into an exteension method~~ +semver:major Made ParseDateTimePermissivelyWithException into an extension method Apr 25, 2025

tombogle added 2 commits April 25, 2025 16:04

Merge branch 'master' into add-buddhist-calendar-tests

3893843

Merge branch 'master' into add-buddhist-calendar-tests

c40af94

imnasnainaec reviewed May 12, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

tombogle added 4 commits May 12, 2025 15:20

Merge branch 'master' into add-buddhist-calendar-tests

195667f

Merge branch 'master' into add-buddhist-calendar-tests

ba3d2fe

Added build step to make hangdump file accessible

0ad2369

tombogle marked this pull request as ready for review May 13, 2025 21:18

tombogle added 2 commits May 13, 2025 17:45

Added timeout for some tests that seem to hang sometimes

f8cdb34

Further tweaks to FileLocationUtilitiesTests to prevent CI failures

804e929

tombogle removed wip - don't merge yet low priority labels May 15, 2025

imnasnainaec reviewed May 15, 2025

View reviewed changes

Deprecate the original version of ParseDateTimePermissivelyWithExcep…

b8fd06b

…tion and added three alternative versions whose names more clearly indicate their behavior. Minor code cleanup in response to code review.

tombogle commented May 15, 2025

View reviewed changes

imnasnainaec reviewed May 15, 2025

View reviewed changes

Minor cleanup

2ec4327

tombogle commented May 16, 2025

View reviewed changes

tombogle merged commit 10e9ac3 into master May 17, 2025
11 of 12 checks passed

tombogle deleted the add-buddhist-calendar-tests branch May 17, 2025 03:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

+semver:major Made ParseDateTimePermissivelyWithException into an extension method#1422

+semver:major Made ParseDateTimePermissivelyWithException into an extension method#1422
tombogle merged 11 commits intomasterfrom
add-buddhist-calendar-tests

tombogle commented Apr 15, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Apr 15, 2025 •

edited

Loading

Uh oh!

imnasnainaec left a comment

Uh oh!

This comment was marked as outdated.

Uh oh!

imnasnainaec left a comment

Uh oh!

tombogle left a comment

Uh oh!

imnasnainaec left a comment

Uh oh!

tombogle left a comment

Uh oh!

tombogle left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

tombogle commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Palaso Tests

Uh oh!

imnasnainaec left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

imnasnainaec left a comment

Choose a reason for hiding this comment

Uh oh!

tombogle left a comment

Choose a reason for hiding this comment

Uh oh!

imnasnainaec left a comment

Choose a reason for hiding this comment

Uh oh!

tombogle left a comment

Choose a reason for hiding this comment

Uh oh!

tombogle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tombogle commented Apr 15, 2025 •

edited

Loading

github-actions bot commented Apr 15, 2025 •

edited

Loading