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

config inMemory #280

Merged
merged 15 commits into from
Mar 7, 2022
Merged

config inMemory #280

merged 15 commits into from
Mar 7, 2022

Conversation

desistefanova
Copy link
Contributor

Implemented inMemory property of realm configuration. Fixes #91

@cla-bot cla-bot bot added the cla: yes label Feb 28, 2022
@coveralls
Copy link

coveralls commented Feb 28, 2022

Pull Request Test Coverage Report for Build 1945207203

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.366%

Totals Coverage Status
Change from base Build 1909825077: 0.0%
Covered Lines: 380
Relevant Lines: 407

💛 - Coveralls

Copy link
Contributor

@nielsenko nielsenko left a comment

Choose a reason for hiding this comment

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

I would prefer Configuration to be immutable, ie. to drop the public setters, so that everything must be set on construction. We hold a reference to the Configuration from a Realm. You cannot really manipulate the Configuration after you have opened the Realm, so why allow stuff like realm.Configuration.inMemory = true in the type system (even if it does throw an exception).

@nielsenko
Copy link
Contributor

I would prefer Configuration to be immutable, ie. to drop the public setters, so that everything must be set on construction. We hold a reference to the Configuration from a Realm. You cannot really manipulate the Configuration after you have opened the Realm, so why allow stuff like realm.Configuration.inMemory = true in the type system (even if it does throw an exception).

On round-table today, we decided to merge as is, and defer making Configuration immutable to later

@nielsenko nielsenko closed this Mar 1, 2022
@nielsenko nielsenko reopened this Mar 1, 2022
Co-authored-by: Kasper Overgård Nielsen <kasper.nielsen@mongodb.com>
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -39,14 +39,19 @@ class Configuration {
/// [readOnly] controls whether a [Realm] is opened as readonly.
/// This allows opening it from locked locations such as resources,
/// bundled with an application. The realm file must already exists.
Configuration(List<SchemaObject> schemaObjects, {bool readOnly = false})
///
/// If [inMemoryOnly] is set, then the [Realm] instance will not be persisted, and hence be _deleted_ after it is closed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think other SDKs have different doc here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please check this comment: #280 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

add a review with a suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Co-authored-by: blagoev <lubo@blagoev.com>
lib/src/configuration.dart Outdated Show resolved Hide resolved
/// Gets or sets a value specifying settings for an in-memory Realm.
/// When all in-memory instance of Realm is closed all data in that Realm is deleted.
bool get inMemory => realmCore.getConfigInMemory(this);
set inMemory(bool value) => realmCore.setConfigInMemory(this, value);
Copy link
Contributor

Choose a reason for hiding this comment

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

This name diverges with isReadOnly we could syncing it. So isInMemory or we rename isReadOnly to readOnly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to isInMemory .

lib/src/configuration.dart Outdated Show resolved Hide resolved
lib/src/configuration.dart Outdated Show resolved Hide resolved
expect(Realm.existsSync(config.path), false);
});

test('Configuration inMemory - lost objects after closing', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could delete this test. Does not make much sense to test that if it is inMemory only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted.

realm.close();
});

test('Configuration inMemory and readOnly', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is testing readonly instead of inMemory. I suggest delete it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted.

lib/src/configuration.dart Show resolved Hide resolved
desistefanova and others added 3 commits March 7, 2022 12:37
Co-authored-by: blagoev <lubo@blagoev.com>
Co-authored-by: blagoev <lubo@blagoev.com>
lib/src/configuration.dart Outdated Show resolved Hide resolved
lib/src/configuration.dart Outdated Show resolved Hide resolved
@desistefanova desistefanova merged commit e5d6e54 into master Mar 7, 2022
@desistefanova desistefanova deleted the config_in_memory branch March 7, 2022 13:03
@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Config: Support inMemory Realm
4 participants