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

[feature] object_properties are a BTreeMap instead of BTreeSet #763

Merged
merged 1 commit into from
May 7, 2021

Conversation

woshilapin
Copy link
Contributor

@woshilapin woshilapin commented Apr 9, 2021

Copy link
Contributor

@pbougue pbougue left a comment

Choose a reason for hiding this comment

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

This looks OK.

Although we might have silent behavior changes, but I'm not sure how to detect them all: check .insert() returns None inside insert_object_property?

I'm not sure that it's an issue either: did not find doc specifying how the conflict should be resolved.

EDIT:
Maybe here https://github.com/CanalTP/transit_model/blob/9d0afbd9957dcef17c66bae2a63bf4bcc53e5ddc/src/ntfs/read.rs#L563
do a entry().or_insert() ?

Cargo.toml Show resolved Hide resolved
src/objects.rs Show resolved Hide resolved
@woshilapin
Copy link
Contributor Author

Some details about most of the comments. If I'm summarizing, the main concern is what do we want between these 2 behaviors:

  • when a duplicate key is to be inserted, we discard it, first one should win
  • when a duplicate key is to be inserted, we just insert it, last one should win

After some more discussions about that topic, we decided that it does not matter that much which one is chosen, as long as it is consistent. Therefore, since last one should win is the default strategy for BTreeMap, we'll keep it as it is, this will avoid some surprising syntax like

properties_map.entry(some_key).or_insert(some_value);

instead of the usually more expected

properties_map.insert(some_key, some_value);

@woshilapin woshilapin marked this pull request as ready for review April 29, 2021 14:43
patochectp
patochectp previously approved these changes May 3, 2021
@patochectp patochectp mentioned this pull request May 3, 2021
@patochectp patochectp requested a review from pbougue May 5, 2021 10:40
pbougue
pbougue previously approved these changes May 5, 2021
@patochectp patochectp dismissed stale reviews from pbougue and themself via 6eaa42e May 7, 2021 06:37
@patochectp patochectp merged commit 0c37fe3 into master May 7, 2021
@patochectp patochectp deleted the object-properties-btreemap branch May 7, 2021 06:53
@pbougue pbougue changed the title [features] object_properties are a BTreeMap instead of BTreeSet [feature] object_properties are a BTreeMap instead of BTreeSet May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants