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

Revisit need of Map.Update<Type> and Map.Insert<Type> #5998

Closed
dmitryax opened this issue Aug 30, 2022 · 5 comments
Closed

Revisit need of Map.Update<Type> and Map.Insert<Type> #5998

dmitryax opened this issue Aug 30, 2022 · 5 comments

Comments

@dmitryax
Copy link
Member

dmitryax commented Aug 30, 2022

We need to make sure that all the pdata API is needed before declaring 1.0.

Problem with Map.Update<Type> and Map.Insert<Type> is that they cannot be applied to all the types. Map.Upsert... methods that require returning a value (e.g. Map.UpsertEmpty or Map.UpsertEmptyMap) cannot have clean and useful equivalents for Map.Update... and Map.Insert....

Usage of Update... in contrib is very limited. There are only few places where it's actually needed. Most of Insert... usages in contrib can be replaced with Upsert... since there is no data under the key.

All the use cases when Update... and Insert... are really needed can be replaced with a combination of Get and Upsert....

Removing Update... and Insert... helps us to keep the Map API concise and consistent. Upsert... methods can be potentially renamed to Put....

@bogdandrutu
Copy link
Member

Map.Update<Type> where marked as deprecated and will be removed next release.

@dmitryax
Copy link
Member Author

dmitryax commented Sep 7, 2022

@bogdandrutu Looks like we are left with 11 Insert occurrences in contrib that cannot be replaced without changing the functionality.

It seems a small enough number to replace them with Get->Upsert. WDYT?

@bogdandrutu
Copy link
Member

@dmitryax agreed, for the moment does not seem that important to offer "Insert" like functionality. Let's deprecate, and we should finalize the name for "Upsert" since that confused lots of people (everyone was using "Insert" as default)

@bogdandrutu
Copy link
Member

@dmitryax only one task left, to find a better name than "Upsert". I think we can close this and open a new issue for that.

@dmitryax
Copy link
Member Author

@bogdandrutu Submitted #6059

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

No branches or pull requests

2 participants