-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add missing toPersistentMap, rename persistentXxxOf to emptyPersistentXxx #177
base: master
Are you sure you want to change the base?
Conversation
Additionally, prevented replaced-withs from ignoring type-paremeters, and added some gradle properties as the library didn't build.
@qurbonzoda are you intending on merging this as a part of the beta? locally, all tests ran; the changes to seemingly all files is just replacing usage of |
The standard library has both |
Please feel free to commit it in a separate PR or in a separate commit within this PR. |
I can't reproduce this issue. Could you please clarify what IDE and its version you are using? |
Could you please add them in a separate commit with description of the fix? |
Calling the persistent version is an implementation detail. We could return a different collection that implements |
Could you please clarify why this might be useful? |
* | ||
* @since 0.4 | ||
*/ | ||
public fun <T, R> Sequence<Pair<T, R>>.toPersistentMap(): PersistentMap<T, R> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add tests for the new functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really; Idk how the current functions are tested, so I can't know how you want the tests to be done. I could not find these tests myself.
Eh, it's more of it wasn't written properly (missing generics and
Sure.
Well no, I've already committed, but:
Will do.
To redirect new users to the correct functions. |
Additionally, prevented replaced-withs from ignoring type-paremeters, and added some gradle properties as the library didn't build.
I've encountered some annyoances when using KotlinX Coroutines, and so I've decided to move my own scattered-around solutions into the library itself.
persistentXxxOf
instead ofemptyXxx
.list.toMap().toImmutableMap()
orpersistentMapOf<Xxx>().mutate { addAll(list) }
.immutableXxxOf
withpersistentXxxOf
results in bugged code, as the type parameter is ignored.Though I do have some questions:
emptyImmutableXxx()
no-op functions be created, similarly toFlow.forEach { }
?.toImmutableMap()
functions be removed or error-deprecated? As far as I understand,Map.toImmutableMap
is kept as it returns the receiver if it is an instance ofImmutableMap
; But the new functions simply call their persistent versions.kotlin-js-store/yarn.lock
be added to the.gitignore
, or should it be committed?Sorry for the big PR; Lots of the changes I did were in the same file and some even in the same lines, so it made more sense to submit them together than separately.