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.

Sign up for GitHub

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

Jump to bottom

Added ApplicationConfiguration #306

Merged
merged 13 commits into from
Apr 12, 2022
Merged

Added ApplicationConfiguration #306

merged 13 commits into from
Apr 12, 2022

Conversation

nielsenko
Copy link
Contributor

@nielsenko nielsenko commented Mar 23, 2022 •
edited by desistefanova
Loading

Implement ApplicationConfiguration basic properties like: base url, local app name, local app version, default timeout, platform, platform version, sdk version. Fixes #401

@cla-bot cla-bot bot added the cla: yes label Mar 23, 2022
@nielsenko nielsenko force-pushed the http_transport branch 2 times, most recently from 46bdc2b to 9ed782c Compare March 24, 2022 09:38
@nielsenko nielsenko force-pushed the realm_app_config branch 2 times, most recently from cd97306 to 6110de1 Compare March 25, 2022 08:22
@nielsenko nielsenko marked this pull request as ready for review March 30, 2022 06:38
@nielsenko
Copy link
Contributor Author

Will rebase on master once current base is merged

nirinchev
nirinchev reviewed Apr 1, 2022
View reviewed changes
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

The PR looks fine, but I'm not sure I understand why we're only implementing the configuration halfway? As far as I can tell, we're missing the 3 metadata properties - base path, encryption key, and persistence mode. Those are just primitive types, so unless there's a strong argument against, I'd suggest we just add them.

@nielsenko
Copy link
Contributor Author

The PR looks fine, but I'm not sure I understand why we're only implementing the configuration halfway? As far as I can tell, we're missing the 3 metadata properties - base path, encryption key, and persistence mode. Those are just primitive types, so unless there's a strong argument against, I'd suggest we just add them.

As is, I only included the properties that are exposed with realm_app_config_set_* functions in realm.h. The properties you refer to, are part of the sync configuration, ie. exposed with realm_sync_client_config_set_*.

I like the idea of combining them into ApplicationConfiguration on the dart side, but perhaps we should discuss that in a wider group?

@nirinchev
Copy link
Member

To create an app we need both an app config and sync client config. Even though they are split in the C API there's no reason to split them in the SDK API and I believe every SDK exposes them on the app config object.

@nielsenko
Copy link
Contributor Author

To create an app we need both an app config and sync client config. Even though they are split in the C API there's no reason to split them in the SDK API and I believe every SDK exposes them on the app config object.

Updated PR as suggested

@nielsenko nielsenko force-pushed the realm_app_config branch 2 times, most recently from 0bba38e to 251229d Compare April 3, 2022 14:07
nirinchev
nirinchev requested changes Apr 4, 2022
View reviewed changes
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

Some docs comments + we seem to be missing the method to create sync config handle.

lib/src/application_configuration.dart Outdated Show resolved Hide resolved
lib/src/application_configuration.dart Outdated Show resolved Hide resolved
lib/src/application_configuration.dart Outdated Show resolved Hide resolved
lib/src/application_configuration.dart Outdated Show resolved Hide resolved
lib/src/application_configuration.dart Outdated Show resolved Hide resolved
lib/src/native/realm_core.dart Outdated Show resolved Hide resolved
@nielsenko nielsenko requested a review from nirinchev April 7, 2022 11:05
@nielsenko nielsenko force-pushed the http_transport branch 2 times, most recently from 07270f6 to c293770 Compare April 7, 2022 16:22
Base automatically changed from http_transport to master April 11, 2022 18:23
@blagoev blagoev merged commit ceb4ed5 into master Apr 12, 2022
@blagoev blagoev deleted the realm_app_config branch April 12, 2022 06:35
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nirinchev nirinchev nirinchev requested changes

@desistefanova desistefanova desistefanova approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

App: support basic properties
4 participants
@nielsenko @nirinchev @desistefanova @blagoev