Remove TryFrom/Into conversion of HashMap, and implement into_hashmap() and from_hashmap() #254

yutannihilation · 2021-08-08T09:35:29Z

Close #230

<List>::from_hashmap()
<List>::into_hashmap()
~~<HashMap<>>::from_list()~~
~~<HashMap<>>::into_list()~~

andy-thomason · 2021-10-14T09:51:40Z

Do you need some help with this?

…hmap

yutannihilation · 2021-10-17T15:15:51Z

@andy-thomason
Thanks, my problems are mainly

I don't keep up with the recent progress of TryFrom conversions. I think I can make it eventually, but feel free to take this over.
To implement the latter half of the goals of this pull request, <HashMap<String, T>>::{from_list, into_list}, I need to figure out the convention of the traits, which will be determined on Design document for Traits #306. So, I'm wondering if I should wait and create a separate pull request.

yutannihilation · 2021-10-19T14:46:10Z

I think this is ready for review now. I removed many integrated tests related to List because this is no longer a conversion that happens between R session and Rust code, so I think unit tests are enough.

clauswilke

Looks good to me in principle. I have a few minor comments.

clauswilke · 2021-10-19T16:29:07Z

extendr-api/src/wrapper/list.rs

+    ///     assert_eq!(names, vec!["a", "b"]);
+    /// }
+    /// ```
+    pub fn from_hashmap<K>(val: HashMap<K, Robj>) -> Result<Self>


Should this take a reference? Otherwise, maybe mention in line 66 that the HashMap is consumed?

Maybe I am missing something, but why not make it a TryFrom implementation?

@clauswilke
I think the result List should own Robjs in the HashMap, so HashMap should be consumed.

@Ilia-Kosenkov
Because the conversion is lossy. Please refer to #230 (comment) for the related discussions.

@yutannihilation That's fine. Then I'd say so explicitly in the docstring.

OK I see. If we do not have it (or have it planned), it may be useful to have trait for lossy conversions instead of multiplying from_name and to_name functions.

clauswilke · 2021-10-19T16:30:22Z

extendr-api/src/wrapper/list.rs

+
+    /// Convert a List into a HashMap, consuming the list.
+    ///
+    /// - If there are some duplicated name of elements, only one of those will be preserved.


clauswilke · 2021-10-19T16:30:53Z

extendr-api/src/wrapper/list.rs

+    /// Convert a List into a HashMap, consuming the list.
+    ///
+    /// - If there are some duplicated name of elements, only one of those will be preserved.
+    /// - If an element doesn't have the name, an empty string (i.e. `""`) will be the key.


"doesn't have a name, ... will be used as the key"

Also mention what happens when there are multiple elements without name?

clauswilke · 2021-10-19T16:32:03Z

tests/extendrtests/tests/testthat/test-type-conversion.R

-  expect_error(list_str_hash(20:30), "expected a list")
-  expect_error(list_str_hash(NA), "expected a list")
-  expect_error(list_str_hash(e), "expected a list")
+  # TODO


Can we add some tests here before merging, or is there a reason to defer?

This TODO doesn't mean we need to add some tests for list. As I commented above, I intentionally removed integrated tests because this is no longer what happens inside the [#extendr] macro, so I think unit tests are enough.

If this comment is confusing, I'll remove this.

clauswilke · 2021-10-19T16:32:45Z

tests/extendrtests/tests/testthat/test-type-conversion.R

-  expect_error(try_list_str_hash(20:30), "Expected List got Integer")
-  expect_error(try_list_str_hash(NA), "Expected List got Logical")
-  expect_error(try_list_str_hash(e), "Expected List got Environment")
+  # TODO


Ilia-Kosenkov · 2021-10-20T08:40:11Z

CHANGELOG.md

+- Remove `TryFrom` conversions between `Robj` and `HashMap` for consistency. Use explicit conversions by `List::into_hashmap()` and `List::from_hashmap()` instead.
+


I think changelog should be in past simple. I suggest to slightly rephrase it to
"Removed TryFrom conversions between Robj and HashMap for consistency. List::into_hashmap() and List::from_hashmap() should be used instead."

…hmap

yutannihilation · 2021-10-20T15:04:25Z

Thanks, I addressed the comments.

yutannihilation · 2021-11-22T23:05:37Z

@andy-thomason
Can I merge this?

yutannihilation added 2 commits August 8, 2021 17:45

Move implementation to List::from_hashmap()

6c29e44

Move implementation to List::into_hashmap()

48ebc51

yutannihilation added 4 commits October 17, 2021 21:33

Merge remote-tracking branch 'upstream/master' into fix/issue-230-has…

4fdaa09

…hmap

Add mut

d890094

Remove unnecessary into()

675501c

Tweak tests

8de2a92

yutannihilation added 2 commits October 18, 2021 22:59

Remove List-related tests from integrated tests

86736ad

Remove another test

cdfc6ed

yutannihilation changed the title ~~Remove TryFrom/Into conversion of HashMap, and implement into_hashmap() and from_hashmap()~~ Remove TryFrom/Into conversion of HashMap Oct 19, 2021

yutannihilation changed the title ~~Remove TryFrom/Into conversion of HashMap~~ Remove TryFrom/Into conversion of HashMap, and implement into_hashmap() and from_hashmap() Oct 19, 2021

Add a changelog [skip ci]

d0f8476

yutannihilation requested review from andy-thomason and Ilia-Kosenkov October 19, 2021 14:44

clauswilke reviewed Oct 19, 2021

View reviewed changes

Ilia-Kosenkov reviewed Oct 20, 2021

View reviewed changes

yutannihilation added 5 commits October 20, 2021 23:44

Merge remote-tracking branch 'upstream/master' into fix/issue-230-has…

ee6dfa6

…hmap

Improve the document of from_hashmap()

d06d03f

name -> names

1da8f2b

Improve the document of into_hashmap()

a11c6b4

Improve CHANGELOG

925de99

clauswilke approved these changes Oct 20, 2021

View reviewed changes

Ilia-Kosenkov approved these changes Oct 21, 2021

View reviewed changes

andy-thomason merged commit f1ce194 into extendr:master Nov 23, 2021

yutannihilation deleted the fix/issue-230-hashmap branch January 23, 2023 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove TryFrom/Into conversion of HashMap, and implement into_hashmap() and from_hashmap() #254

Remove TryFrom/Into conversion of HashMap, and implement into_hashmap() and from_hashmap() #254

yutannihilation commented Aug 8, 2021 •

edited

andy-thomason commented Oct 14, 2021

yutannihilation commented Oct 17, 2021

yutannihilation commented Oct 19, 2021

clauswilke left a comment

clauswilke Oct 19, 2021

Ilia-Kosenkov Oct 20, 2021

yutannihilation Oct 20, 2021

clauswilke Oct 20, 2021

Ilia-Kosenkov Oct 20, 2021

clauswilke Oct 19, 2021

clauswilke Oct 19, 2021

clauswilke Oct 19, 2021

yutannihilation Oct 20, 2021

clauswilke Oct 19, 2021

Ilia-Kosenkov Oct 20, 2021

yutannihilation commented Oct 20, 2021

yutannihilation commented Nov 22, 2021

		- Remove `TryFrom` conversions between `Robj` and `HashMap` for consistency. Use explicit conversions by `List::into_hashmap()` and `List::from_hashmap()` instead.

Remove TryFrom/Into conversion of HashMap, and implement into_hashmap() and from_hashmap() #254

Remove TryFrom/Into conversion of HashMap, and implement into_hashmap() and from_hashmap() #254

Conversation

yutannihilation commented Aug 8, 2021 • edited

andy-thomason commented Oct 14, 2021

yutannihilation commented Oct 17, 2021

yutannihilation commented Oct 19, 2021

clauswilke 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

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

yutannihilation commented Oct 20, 2021

yutannihilation commented Nov 22, 2021

yutannihilation commented Aug 8, 2021 •

edited