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

@W-15363709 Changes to RestAPI for modern Async/Await #3806

Merged
merged 9 commits into from
Jan 24, 2025

Conversation

Crebs
Copy link
Contributor

@Crebs Crebs commented Jan 16, 2025

No description provided.

@Crebs Crebs self-assigned this Jan 16, 2025
@Crebs Crebs requested review from bbirman and wmathurin January 16, 2025 19:38
Copy link
Contributor

@wmathurin wmathurin left a comment

Choose a reason for hiding this comment

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

Shouldn't we keep the tests for the old APIs since they are deprecated and not (yet) removed?

@bbirman
Copy link
Member

bbirman commented Jan 23, 2025

👍 for getting rid of the warnings in NativeLoginManager. I still see two in RestAPIExplorer (ignore the traitCollection one)
rest api explorer warnings

@Crebs Crebs requested a review from bbirman January 23, 2025 20:45
self.handleError(request: request, error: error)
}

self.view.endEditing(true)
Copy link
Member

@bbirman bbirman Jan 23, 2025

Choose a reason for hiding this comment

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

what's this change for? is the same call at line 562 still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, that was a copy error. I take that out. Good catch on that one. 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bbirman OK, Fixed my goof. Thanks for catching that .

Copy link
Member

@bbirman bbirman left a comment

Choose a reason for hiding this comment

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

lgtm! It looks like one of the templates uses the old way too so please update that as well, let me know if you have any questions about that repo!
https://github.com/forcedotcom/SalesforceMobileSDK-Templates/blob/358f7cace478f6458c32f57283e8364e493b8cd5/iOSNativeSwiftEncryptedNotificationTemplate/EncryptedNotificationTemplate/RootViewController.swift#L39

@Crebs Crebs merged commit 279c9a2 into forcedotcom:dev Jan 24, 2025
4 of 6 checks passed
@Crebs Crebs deleted the u/rcrebs/@W-15363709 branch January 24, 2025 18:01
@Crebs
Copy link
Contributor Author

Crebs commented Jan 24, 2025

@bbirman I made update to the template repo. PR is here forcedotcom/SalesforceMobileSDK-Templates#441

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.

3 participants