Skip to content

Conversation

suhaibabsi-inst
Copy link
Contributor

This is to fix issues preventing builds compiling on Xcode 16

Checklist

  • Follow-up e2e test ticket created
  • A11y checked
  • Tested on phone
  • Tested on tablet
  • Tested in dark mode
  • Tested in light mode
  • Approve from product

@suhaibabsi-inst suhaibabsi-inst marked this pull request as ready for review September 24, 2024 09:16
@inst-danger
Copy link
Contributor

inst-danger commented Sep 24, 2024

Warnings
⚠️ One or more files are below the minimum test coverage 50%
Coverage New % Master % Delta
Canvas iOS 91.08% 91.17% -0.09%
Core/Core/Planner/CalendarEvent/Model/API/PostCalendarEventRequest.swift 0% 0% 0%
Core/Core/Planner/CalendarEvent/View/CustomFrequencyComponents/OccurrencesCountInputDialog.swift 0% 0% 0%
Core/Core/Planner/CalendarEvent/Model/Helpers/RecurrenceRule+SelectionDescription.swift 43.33% 43.33% 0%
Core/Core/Grades/View/CustomSwipeAction.swift 0% 0% 0%

Generated by 🚫 dangerJS against 82436b3

@suhaibabsi-inst suhaibabsi-inst self-assigned this Sep 24, 2024
@szabinst
Copy link
Collaborator

When trying to build for iOS 18 simulator, I get a crash. Simulators with < iOS 18.0 works well.

Screenshot 2024-09-25 at 10 05 21

@suhaibabsi-inst
Copy link
Contributor Author

@szabinst

Updating PSDPDFKit to version 13.9.1 to resolves the issue, while we aim with it to keep supporting pre Xcode 16 for those who still need more time to update.
Once, all the team is on Xcode 16, I think we should pump this one to 14.0.0 for better support as per their change log.

szabinst
szabinst previously approved these changes Sep 25, 2024
@szabinst
Copy link
Collaborator

@suhaibabsi-inst Please remove affects, etc.. commit message and use [ignore-commit-lint] so danger succeeds.

Copy link
Contributor

@rh12 rh12 left a comment

Choose a reason for hiding this comment

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

Built & run successfully on Xcode15.4.
Test build failed, please see comment below.
We could either work around this somehow, or revert this change and accept that Xcode16 test builds will fail until the whole team updates to Xcode in the near future.

[ignore-commit-lint]
@suhaibabsi-inst
Copy link
Contributor Author

Will be waiting to merge this until the release is cut.

@vargaat vargaat requested a review from ndrsszsz as a code owner October 21, 2024 12:55
Copy link
Contributor

@rh12 rh12 left a comment

Choose a reason for hiding this comment

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

Code review +1

@@ -40,5 +40,27 @@ public extension HTTPCookie {
)!
}

/// The default equality check returns false even when all properties match
/// so we created this custom comparison to test property equality between this and another cookie.
public func equalsProperties(to anotherCookie: HTTPCookie?) -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public func equalsProperties(to anotherCookie: HTTPCookie?) -> Bool {
public func isEqualProperties(to anotherCookie: HTTPCookie?) -> Bool {

szabinst
szabinst previously approved these changes Oct 24, 2024
Copy link
Collaborator

@szabinst szabinst left a comment

Choose a reason for hiding this comment

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

LGTM on both simulator, physical device.

…moving the custom class name from the database model.
rh12
rh12 previously approved these changes Oct 24, 2024
Copy link
Contributor

@rh12 rh12 left a comment

Choose a reason for hiding this comment

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

QA + 1
Built & run on iOS 17 (iPad, iPhone, iPhone simulator)
All 3 apps seem to work as before.

@vargaat vargaat merged commit 17f75e0 into master Oct 24, 2024
3 of 4 checks passed
@vargaat vargaat deleted the bugfix/Xcode16-Fixes branch October 24, 2024 11:30
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.

5 participants