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 default constructor for MapRef #4139

Merged
merged 8 commits into from
Nov 21, 2024

Conversation

yisraelU
Copy link
Contributor

Adds an apply method for MapRef. If the Effect type is an instance of Async , the ConcurrentHashMap implementation will be used otherwise the sharedImmutableMap will be used with the number of shards set to the number of Processors determined by Runtime.getRuntime.availableProcessors.
The goal of this MR is to improve the UX for a user by having a sane default

closes #4136

P.S. as this is a first-time PR to this repo, please let me know if there are any conventions/practices, etc.. that I should be adhering to. TY

Copy link
Contributor

@BalmungSan BalmungSan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this alone is great.
We may also request a default value in apply to improve UX as suggested by @Jasper-M

package-lock.json Outdated Show resolved Hide resolved
std/shared/src/main/scala/cats/effect/std/MapRef.scala Outdated Show resolved Hide resolved
std/shared/src/main/scala/cats/effect/std/MapRef.scala Outdated Show resolved Hide resolved
std/shared/src/main/scala/cats/effect/std/MapRef.scala Outdated Show resolved Hide resolved
update type check to Sync and address feedback
@yisraelU
Copy link
Contributor Author

While this alone is great. Wwe may also request a default value in apply to improve UX as suggested by @Jasper-M

I have added an overloaded apply . WDYT

@BalmungSan
Copy link
Contributor

BalmungSan commented Sep 17, 2024

I have added an overloaded apply . WDYT

Sadly I don't think that would work well: https://scastie.scala-lang.org/BalmungSan/WHoq2ShkTCm3QSs5puHixw

I think the best would be to do what Arman suggested and rather add a withDefault (better names are welcome) extension method to MapRefOptionOps. So that users can do:

val m1 = MapRef[IO, K, V] // Returns MapRef[IO, K, Option[V]
val m2 = m1.withDefault(foo) // Returns MapRef[IO, K, V]

@yisraelU
Copy link
Contributor Author

yisraelU commented Sep 17, 2024

I have added an overloaded apply . WDYT

Sadly I don't think that would work well: https://scastie.scala-lang.org/BalmungSan/WHoq2ShkTCm3QSs5puHixw

I think the best would be to do what Arman suggested and rather add a withDefault (better names are welcome) extension method to MapRefOptionOps. So that users can do:

val m1 = MapRef[IO, K, V] // Returns MapRef[IO, K, Option[V]
val m2 = m1.withDefault(foo) // Returns MapRef[IO, K, V]

I see, it fails for certain types.
If we go back to empty parens for the apply method it should work.
I'm happy with the extension method solution though, and I have nothing better than withDefault :)

@armanbilge armanbilge added this to the v3.6.0 milestone Sep 20, 2024
@armanbilge armanbilge changed the title add apply method for Mapref Add default constructor for MapRef Oct 15, 2024
@armanbilge armanbilge closed this Oct 29, 2024
@armanbilge armanbilge reopened this Oct 29, 2024
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@armanbilge armanbilge merged commit 2d27493 into typelevel:series/3.x Nov 21, 2024
29 of 33 checks passed
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.

Default constructor for MapRef
4 participants