Added overloaded set method for dictionary #5377

papafe · 2023-02-06T14:22:51Z

This closes #4286

☑️ ToDos

📝 Changelog entry
🚦 Tests

papafe · 2023-02-06T14:26:54Z

For the reviewers:

I've left some TODOs in the code with some questions about things I'm not sure about
I tried to avoid duplicating code, but I am not sure mine was the best approach to the overloaded function
Not sure if the docs make sense. Especially with the element input, I am not sure how to explain that it can potentially add multiple key-value pairs
I will add an entry to the changelog once there is an agreement that at least the method definition makes sense 😁

kraenhansen

Minor comments. Great job!

integration-tests/tests/src/tests/dictionary.ts

kraenhansen · 2023-02-06T14:59:23Z

integration-tests/tests/src/tests/dictionary.ts

+      this.realm.write(() => item.dict.remove("key1").remove("key2"));
+
+      expect(Object.keys(item.dict)).deep.equals([]);
+      //expect(item.dict).deep.equals({}); //TODO Not sure why this does not work


I guess it's because dict has instance methods besides the key/value pairs it might have.

I see! Thanks

packages/realm/src/Dictionary.ts

kraenhansen · 2023-02-06T15:05:58Z

packages/realm/src/Dictionary.ts

+    const inputObj = typeof elementOrKey == "string" ? { [elementOrKey]: value } : elementOrKey;
+    for (const [key, val] of Object.entries(inputObj)) {
+      internal.insertAny(key, toBinding(val, undefined));
    }
+    return this;


Great catch .. I also found this on my other PR - let's see who gets to merge first 😄 (although it shouldn't conflict too much).

kraenhansen · 2023-02-06T15:08:23Z

packages/realm/src/ObjectListeners.ts

@@ -44,7 +44,7 @@ export class ObjectListeners<T> {
          });
        } catch (err) {
          // Scheduling a throw on the event loop,
-          // since throwing synchroniously here would result in an abort in the calling C++
+          // since throwing synchronously here would result in an abort in the calling C++


Thanks. Strange I didn't catch these in 789a688 🤔

Co-authored-by: Kræn Hansen <[email protected]>

papafe · 2023-02-06T16:51:41Z

packages/realm/src/Dictionary.ts

+   * @param value The value of the element to add
+   * @throws {@link AssertionError} If not inside a write transaction or if value violates type constraints
+   * @returns The dictionary
+   * @since 10.6.0  //TODO Should also this be 12.0?


I'm not sure about this, also considering this is an overloaded method

I believe it should be 10.6.0 as we introduced Realm.Dictionary in that version

kneth

Great job!

kneth · 2023-02-07T08:43:58Z

packages/realm/src/Dictionary.ts

+   * @param value The value of the element to add
+   * @throws {@link AssertionError} If not inside a write transaction or if value violates type constraints
+   * @returns The dictionary
+   * @since 12.0 //TODO Is this correct?


It will be 12.0.0 (or maybe 12.0.0-rc.0)

kneth · 2023-02-07T08:45:03Z

packages/realm/src/Dictionary.ts

+   * @param value The value of the element to add
+   * @throws {@link AssertionError} If not inside a write transaction or if value violates type constraints
+   * @returns The dictionary
+   * @since 10.6.0  //TODO Should also this be 12.0?


I believe it should be 10.6.0 as we introduced Realm.Dictionary in that version

* bindgen: [bindgen] Migrated `tsm` → `tsx` (#5378) [bindgen] Refactor namespaces (#5376) Fixed space Small correction to docs # Conflicts: # packages/realm/src/Dictionary.ts

takameyer

Just some minor things. Otherwise LGTM!

takameyer · 2023-02-07T14:51:33Z

CHANGELOG.md

@@ -1,7 +1,7 @@
 ## vNext (TBD)

 ### Enhancements
-* None
+* Add an overload to `Dictionary.set` method that takes two arguments, a `key` and a `value`. ([#4286](https://github.com/realm/realm-js/issues/4286))  


Could be useful to provide an example of the new syntax in the changelog for anyone reading this.

Added an example of usage

takameyer · 2023-02-07T14:58:26Z

packages/realm/src/Dictionary.ts

-      internal.insertAny(key, toBinding(value, undefined));
+    const elements = typeof elementsOrKey == "string" ? { [elementsOrKey]: value } : elementsOrKey;
+    for (const [key, val] of Object.entries(elements)) {
+      internal.insertAny(key, toBinding(val, undefined));


Does undefined need to be explicitly set in toBinding? Any argument not defined will be undefined in its method.

Makes sense. This seems to have been done mostly in this commit: e9ce475#diff-90e9134a0cbc40c4c81b2a112040ea96569d8de35ce930f7e3d2f20521a712e7, maybe it was done to make the parameter explicit?

Added overloaded set method for dictionary

d09273d

cla-bot bot added the cla: yes label Feb 6, 2023

papafe requested review from takameyer, kraenhansen and kneth February 6, 2023 14:28

kraenhansen approved these changes Feb 6, 2023

View reviewed changes

papafe and others added 3 commits February 6, 2023 17:35

Update integration-tests/tests/src/tests/dictionary.ts

d1d92e1

Co-authored-by: Kræn Hansen <[email protected]>

Apply suggestions from code review

96a49dc

Co-authored-by: Kræn Hansen <[email protected]>

Small corrections

68049f7

papafe commented Feb 6, 2023

View reviewed changes

kneth approved these changes Feb 7, 2023

View reviewed changes

papafe added 4 commits February 7, 2023 13:55

Corrections according to PR comments

210ba56

Added changelog

1bcb45e

Merge branch 'bindgen' into fp/add-set-overload

2c54ae4

* bindgen: [bindgen] Migrated `tsm` → `tsx` (#5378) [bindgen] Refactor namespaces (#5376) Fixed space Small correction to docs # Conflicts: # packages/realm/src/Dictionary.ts

Fixed merge

78846bd

papafe marked this pull request as ready for review February 7, 2023 13:35

takameyer approved these changes Feb 7, 2023

View reviewed changes

papafe added 2 commits February 7, 2023 17:08

Added example to changelog

aaa1ce8

Merge branch 'bindgen' into fp/add-set-overload

3227f95

papafe merged commit d64f7e3 into bindgen Feb 13, 2023

papafe deleted the fp/add-set-overload branch February 13, 2023 10:45

kraenhansen pushed a commit that referenced this pull request Mar 1, 2023

Added overloaded set method for dictionary (#5377)

f3588d3

kraenhansen pushed a commit that referenced this pull request Mar 3, 2023

Added overloaded set method for dictionary (#5377)

093de79

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added overloaded set method for dictionary #5377

Added overloaded set method for dictionary #5377

papafe commented Feb 6, 2023 •

edited

Loading

papafe commented Feb 6, 2023 •

edited

Loading

kraenhansen left a comment

kraenhansen Feb 6, 2023

papafe Feb 6, 2023

kraenhansen Feb 6, 2023

kraenhansen Feb 6, 2023

papafe Feb 6, 2023

kneth Feb 7, 2023

kneth left a comment

kneth Feb 7, 2023

kneth Feb 7, 2023

takameyer left a comment

takameyer Feb 7, 2023

papafe Feb 7, 2023

takameyer Feb 7, 2023

papafe Feb 7, 2023

Added overloaded set method for dictionary #5377

Added overloaded set method for dictionary #5377

Conversation

papafe commented Feb 6, 2023 • edited Loading

☑️ ToDos

papafe commented Feb 6, 2023 • edited Loading

kraenhansen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kneth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takameyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe commented Feb 6, 2023 •

edited

Loading

papafe commented Feb 6, 2023 •

edited

Loading