-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
RCOCOA-2305: Update base url to point to services.cloud.mongodb.com #8537
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some minor comments
@@ -223,7 +223,7 @@ static void setOptionalString(std::optional<std::string>& dst, NSString *src) { | |||
} | |||
|
|||
- (NSString *)baseURL { | |||
return getOptionalString(_config.base_url); | |||
return getOptionalString(_config.base_url) ?: RLMStringViewToNSString(app::App::default_base_url()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be done by Core, and return the default url if there is no given url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it could, but they haven't done it 😄 I think it's a fair decision on their part as they probably want to keep the struct fairly simple and have the defaults be managed elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea behind returning nil by default here was that most users shouldn't need to care what the base url is so we didn't even want to expose the default. The field in the core struct is optional specifically to support doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that most users shouldn't need to care, I am just not a huge fan of API where there's a default but it's not obvious what that is. I think returning it, even just for sanity checks is cleaner, but am open to being convinced otherwise.
@@ -42,7 +42,7 @@ typedef void(^RLMOptionalErrorBlock)(NSError * _Nullable); | |||
@interface RLMAppConfiguration : NSObject <NSCopying> | |||
|
|||
/// A custom base URL to request against. | |||
@property (nonatomic, strong, nullable) NSString *baseURL; | |||
@property (nonatomic, strong, null_resettable) NSString *baseURL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the docstring should be updated to reflect that now we return the default url if there is no given base url, if we are going in that direction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Looks like there is still some linting issue. Finally, before merging I'll separate this into two commits, one for the fix and one for the core update
* master: RCOCOA-2305: Update base url to point to services.cloud.mongodb.com (#8537) Update docs URL (#8538) RCOCOA-2310: Fix a crash when receiving 401/403 when opening a watch stream (#8536) Add core version to build-binaries workflow (#8525) Add an empty changelog section Upgrade to core 14.4.1 (#8526) Release v10.49.0 Upgrade to core 14 (#8496) RCOCOA-2308: Add GHA workflow for core prebuilds (#8517) Release 10.48.1 Add NSPrivacyAccessedAPICategoryDiskSpace to the privacy manifest (#8511) 🔄 Synced file(s) with realm/ci-actions (#8520) Add build-binaries workflow (#8515) Only commit the changelog in the add empty changelog release step (#8505) Release 10.48.0 Introduce `_customRealmProperties()` to all Realm Objects to allow for client-generated RLMProperties (via Swift Macros) (#8490) Update packaging for Xcode 15.3 (#8502) # Conflicts: # Realm/RLMAsyncTask.mm # Realm/RLMSyncSession.mm
Fixes #8512