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

Implement Date Component Differences and Add Support for Range Queries in Calendar Operations #25

Merged
merged 11 commits into from
Oct 15, 2024

Conversation

shial4
Copy link
Contributor

@shial4 shial4 commented Oct 14, 2024

Here’s the updated PR description to reflect the new changes, which now include additional support for range(of:in:for:), handling of weekOfMonth, weekOfYear, day, and other calendar components:


Summary

This PR resolves the previously missing logic for calculating date component differences between a date and an endDate in the DateComponents initializer and adds support for various date operations. Previously, the initializer contained a fatalError for handling date field differences, causing crashes when using certain date-based operations. Additionally, we now provide support for more calendar component-based operations such as range(of:in:for:) for day, weekOfMonth, weekOfYear, and more.

Changes:

  • Date Difference Calculation: The initializer now calculates differences between date and endDate for components such as era, year, month, day, hour, minute, second, weekday, weekOfMonth, and weekOfYear.
  • Range Support:
    • Added support for range(of:in:for:) with components like day, weekOfMonth, weekOfYear within month and year.
    • For example, range(of: .day, in: .month, for: date) returns the valid range of days in a month.
  • Handling nil endDate: If endDate is not provided, the initializer simply extracts the component values from the given date.
  • Unsupported Fields:
    • Fields unsupported by java.util.Calendar (e.g., nanosecond, quarter, yearForWeekOfYear) have been commented out for future handling.

Testing

  • Verified using my local app, which previously crashed on certain date operations, now performs correctly.
  • Added checks for ranges such as weekOfMonth, weekOfYear, and day for better date component handling.

Impact

  • This update resolves crashes caused by the unimplemented date field difference logic and ensures proper handling of date ranges across the application.

  • The added range support now enables users to query calendar components such as days in a month, weeks in a month, and weeks in a year, improving date-related functionality.

  • REQUIRED: I have signed the Contributor Agreement

  • REQUIRED: I have tested my change locally with swift test

  • OPTIONAL: I have tested my change on an iOS simulator or device

  • OPTIONAL: I have tested my change on an Android emulator or device

Copy link

cla-bot bot commented Oct 14, 2024

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Szymon.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@shial4
Copy link
Contributor Author

shial4 commented Oct 14, 2024

  1. git config --list | grep email

Added my missing email to GitHub account

@marcprux
Copy link
Contributor

This looks great, but can you drop a couple tests for the logic into https://github.com/skiptools/skip-foundation/blob/main/Tests/SkipFoundationTests/DateTime/TestCalendar.swift, so we can be sure that the SkipFoundation version behaves the same as CoreFoundation?

@shial4 shial4 changed the title Implement handling of date field differences for date components Implement Date Component Differences and Add Support for Range Queries in Calendar Operations Oct 15, 2024
@cla-bot cla-bot bot added the cla-signed label Oct 15, 2024
@shial4
Copy link
Contributor Author

shial4 commented Oct 15, 2024

@marcprux
Ok, Added some test also updated title & description as I've made all the changes here for the functionality that I missed to have my calendar component working correctly on Android

@shial4
Copy link
Contributor Author

shial4 commented Oct 15, 2024

Ok, I think Now I'm down. Went tru few iteration but it does match Swift now
Although, when I test there is few unrelated test failures in code not not related to this change, I think.

@marcprux
Copy link
Contributor

Although, when I test there is few unrelated test failures in code not not related to this change, I think.

The TestCalendar.testTimeZoneDates, TestCalendar.testCalendarWithIdentifier, and TestCalendar.testDatesInCESTTimeZone failures are new. Do those fail for you when you run the SkipFoundation tests locally (i.e., against macOS and Robolectric)?

Copy link
Contributor

@marcprux marcprux left a comment

Choose a reason for hiding this comment

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

Looks great overall! Just a couple of review comments and a passing CI run and this can get merged.

Sources/SkipFoundation/DateComponents.swift Outdated Show resolved Hide resolved
Sources/SkipFoundation/Calendar.swift Show resolved Hide resolved
@shial4
Copy link
Contributor Author

shial4 commented Oct 15, 2024

Although, when I test there is few unrelated test failures in code not not related to this change, I think.

The TestCalendar.testTimeZoneDates, TestCalendar.testCalendarWithIdentifier, and TestCalendar.testDatesInCESTTimeZone failures are new. Do those fail for you when you run the SkipFoundation tests locally (i.e., against macOS and Robolectric)?

Regarding the test.
only testCalendarWithIdentifier fails for me with
java.lang.AssertionError: gregorian != iso8601

@shial4 shial4 requested a review from marcprux October 15, 2024 23:04
@marcprux
Copy link
Contributor

only testCalendarWithIdentifier fails for me with
java.lang.AssertionError: gregorian != iso8601

Looks good. I think that assertion can be disabled, because we don't support any calendar other than Gregorian yet. Feel free to just remove the assertion.

@shial4
Copy link
Contributor Author

shial4 commented Oct 15, 2024

only testCalendarWithIdentifier fails for me with
java.lang.AssertionError: gregorian != iso8601

Looks good. I think that assertion can be disabled, because we don't support any calendar other than Gregorian yet. Feel free to just remove the assertion.

done, deal

@marcprux marcprux merged commit 4e395c1 into skiptools:main Oct 15, 2024
2 checks passed
@marcprux
Copy link
Contributor

Tests all pass. Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants