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

remove HashMap from PropertiesList public api #45

Closed
ursusursus opened this issue Jan 31, 2022 · 3 comments
Closed

remove HashMap from PropertiesList public api #45

ursusursus opened this issue Jan 31, 2022 · 3 comments

Comments

@ursusursus
Copy link

Hi, could the PropertiesList please take Map instead of HashMap to be more kotlin idiomatic?

@adam1929
Copy link
Collaborator

adam1929 commented Jan 4, 2023

Sorry @ursusursus API is already defined and we are not able to change it -- technically speaking it is possible and it is easy, but unfortunately it will hit all existing customers :-/

@adam1929 adam1929 closed this as completed Jan 4, 2023
@ursusursus
Copy link
Author

how so? if hashmap extends map, then all callsites should work even if library changes api to map, no?

@adam1929 adam1929 reopened this Jan 5, 2023
@adam1929
Copy link
Collaborator

adam1929 commented Jan 5, 2023

No, this is opposite.
Hashmap extends from Map AND MutableMap. So you may put items to hashmap at any time and that instance will be updated (technically speaking Hashmap has 'set' operator).
Map is "only" map. So you are unable to put items to existing instance, you'll get a new 'map' instance but a original one will not be updated (has only 'plus' operator).
So from this, you may imagine a case that developer will create a var props = PropertiesList(data) and then he'll put items into 'data' rather than 'props'. His result will have same result. If we change it to 'Map', he'll get a compilation error.
But back to original question -> you are right that Map has to be used in these cases, with no doubt. But we already release it as part of SDK API and we are unable to change it because it will hit all existing developers and it forces them to reimplement their part of code (potentially).

@adam1929 adam1929 closed this as completed Jan 5, 2023
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