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

Add missing toPersistentMap, rename persistentXxxOf to emptyPersistentXxx #177

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Laxystem
Copy link

@Laxystem Laxystem commented Apr 3, 2024

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.

  • The functions to create empty collections/maps are named inconsistently with the stdlib; persistentXxxOf instead of emptyXxx.
  • There are no functions to create persistent and immutable maps from sequences, arrays, and iterables of pairs. The only options are to call list.toMap().toImmutableMap() or persistentMapOf<Xxx>().mutate { addAll(list) } .
  • Replacing immutableXxxOf with persistentXxxOf results in bugged code, as the type parameter is ignored.
  • The library didn't build at all on my Linux laptop. I have added some gradle properties to fix that.

Though I do have some questions:

  • Should error-deprecated emptyImmutableXxx() no-op functions be created, similarly to Flow.forEach { }?
  • Should the .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 of ImmutableMap; But the new functions simply call their persistent versions.
  • As a side-note, should 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.

Additionally, prevented replaced-withs from ignoring type-paremeters,
and added some gradle properties as the library didn't build.
@Laxystem Laxystem changed the title Added missing toPersistentMap, renamed persistentXxxOf to Add missing toPersistentMap, rename persistentXxxOf to emptyPersistentXxx Apr 3, 2024
@qurbonzoda qurbonzoda self-requested a review April 5, 2024 09:31
@Laxystem
Copy link
Author

@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 persistentXxxOf with emptyXxx; and the previous functions still exist, but are deprecated; in short, this scary PR really isn't that scary.

@qurbonzoda
Copy link
Contributor

The functions to create empty collections/maps are named inconsistently with the stdlib; persistentXxxOf instead of emptyXxx.

The standard library has both collectionOf() and emptyCollection() variants. For example, listOf() and emptyList(). The same of Set and Map.
Hence, I suggest adding the emptyPersistentCollection() functions in addition to the existing persistentCollectionOf(), not instead of them.

@qurbonzoda
Copy link
Contributor

qurbonzoda commented Nov 3, 2024

should kotlin-js-store/yarn.lock be added to the .gitignore, or should it be committed?

Please feel free to commit it in a separate PR or in a separate commit within this PR.

@qurbonzoda
Copy link
Contributor

Replacing immutableXxxOf with persistentXxxOf results in bugged code, as the type parameter is ignored.

I can't reproduce this issue. Could you please clarify what IDE and its version you are using?

@qurbonzoda
Copy link
Contributor

The library didn't build at all on my Linux laptop. I have added some gradle properties to fix that.

Could you please add them in a separate commit with description of the fix?

@qurbonzoda
Copy link
Contributor

Should the .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 of ImmutableMap; But the new functions simply call their persistent versions.

Calling the persistent version is an implementation detail. We could return a different collection that implements ImmutableMap but not PersistentMap, which might have a better performance.

@qurbonzoda
Copy link
Contributor

Should error-deprecated emptyImmutableXxx() no-op functions be created, similarly to Flow.forEach { }?

Could you please clarify why this might be useful?

*
* @since 0.4
*/
public fun <T, R> Sequence<Pair<T, R>>.toPersistentMap(): PersistentMap<T, R> =
Copy link
Contributor

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?

Copy link
Author

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.

@Laxystem
Copy link
Author

Laxystem commented Nov 8, 2024

Replacing immutableXxxOf with persistentXxxOf results in bugged code, as the type parameter is ignored.

I can't reproduce this issue. Could you please clarify what IDE and its version you are using?

Eh, it's more of it wasn't written properly (missing generics and this), and I had expected (and experienced some I forgot) bugs when having multiple lists of the same type in context, complex generics, etc.

Hence, I suggest adding the emptyPersistentCollection() functions in addition to the existing persistentCollectionOf(), not instead of them.

Sure.

The library didn't build at all on my Linux laptop. I have added some gradle properties to fix that.

Could you please add them in a separate commit with description of the fix?

Well no, I've already committed, but:

  1. The benchmarks one I don't remember. I assume the default template was applied when it shouldn't've been, which caused a crash.
  2. The second one, is that you cannot build Apple targets on Linux and Windows machines, so I've added a property that skips them in such scenario.

should kotlin-js-store/yarn.lock be added to the .gitignore, or should it be committed?

Please feel free to commit it in a separate PR or in a separate commit within this PR.

Will do.

Should error-deprecated emptyImmutableXxx() no-op functions be created, similarly to Flow.forEach { }?

Could you please clarify why this might be useful?

To redirect new users to the correct functions.

@Laxystem Laxystem reopened this Nov 8, 2024
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.

2 participants