Add missing toPersistentMap, rename persistentXxxOf to emptyPersistentXxx #177
Conversation
Additionally, prevented replaced-withs from ignoring type-paremeters, and added some gradle properties as the library didn't build.
Laxystem
changed the title
|
@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.
Could you please add tests for the new functions?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Map tests are mostly in this file: https://github.com/Kotlin/kotlinx.collections.immutable/blob/master/core/commonTest/src/contract/map/ImmutableMapTest.kt
You can add tests for new functions either there or in a new file within the directory.
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.
You can always use Git commands, such as reset and interactive rebase, to reorganize your commits. Please note that for the PR to be merged, each separate change should be in a separate commit. For example, there should be one commit for introducing the If you find reorganizing commits difficult, please let me know. I can do it before merging. |
| @@ -35,7 +36,7 @@ open class AddAll { | |||
| */ | |||
| @Benchmark | |||
| fun addAllLast(): ImmutableList<String> { | |||
| return persistentListOf<String>().addAll(listToAdd) | |||
| return emptyPersistentList<String>().addAll(listToAdd) | |||
There was a problem hiding this comment.
Given persistentCollectionOf functions are not deprecated, could you please revert all changes in the PR that replace persistentCollectionOf usages?
There was a problem hiding this comment.
I could; But isn't it clearer this way?
There was a problem hiding this comment.
I believe it's a matter of preference, and not worth modifying a lot of files.
|
@qurbonzoda could you please implement all the changes you've detailed? I'm sorry, but I can't find time to this myself—I'd love to, sorry. |
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.
persistentXxxOfinstead ofemptyXxx.list.toMap().toImmutableMap()orpersistentMapOf<Xxx>().mutate { addAll(list) }.immutableXxxOfwithpersistentXxxOfresults 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.toImmutableMapis 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.lockbe 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.