-
Notifications
You must be signed in to change notification settings - Fork 508
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
Feature/prop #3924
base: series/3.x
Are you sure you want to change the base?
Feature/prop #3924
Conversation
Thank you for looking at this! An interesting question for us to answer though is what we want to do with Native and JavaScript. This is a bit different from |
They both support System properties. It's basically just a global mutable map. |
A question: the |
I would agree. TBH, I wondered whether we should try and make them atomic, but that's probably not something you'd expect by system props. |
I don't think we can make them atomic. Or do you know a way? |
Right, not sure how could be made atomic. Citing Arman, it's basically just a global mutable map. |
Unfortunately I think it's still confusing. Anyway, I expect 99% of usecases to just be reading system properties. APIs that require setting/updating system properties seem really dicey 🙄 |
I'd just go with get, set and remove. If that's the functionality offered by the platform, and we can't provide atomicity, its best simply to wrap those. |
Similar to |
*/ | ||
def unset(key: String): F[Unit] | ||
|
||
def entries: F[Properties] |
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.
I'm not sure I like returning a mutable object here. (I think we usually try to avoid that?)
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.
For Env
we decided to return an Iterable[(String, String)]
, but that was because of case-sensitivity issues which I don't think apply here. So probably makes most sense to return an immutable Map[String, String]
.
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.
I think the entries
is not an atomic snapshot of the system properties. it would be nice if we could do that, but I don't think it's possible with the available JDK API.
F.delay(Option(System.getProperty(key))) // thread-safe | ||
|
||
def set(key: String, value: String): F[Unit] = | ||
F.void(F.delay(System.setProperty(key, value))) |
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.
If I'm reading the JDK source correctly, this eventually calls a synchronized
method. So, technically, it should be F.blocking
. I don't know if we care about this though...
@nowarn3("cat=deprecation") | ||
def entries: F[Map[String, String]] = { | ||
import scala.collection.JavaConverters._ | ||
F.delay(Map.empty ++ System.getProperties().asScala) |
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.
Similarly, I think this calls into a Collections.synchronizedSet
, so technically should be blocking
. (Although this should be uncontended, I think. So we probably care even less...)
for { | ||
_ <- Prop[IO].set("some property", "the value") | ||
props <- Prop[IO].entries | ||
assertion <- IO(props mustEqual System.getProperties()) |
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.
There is a test failure here, I think it's just because we're comparing a Scala Map
with a Java hashtable.
|
||
import scala.collection.immutable.Map | ||
|
||
trait Prop[F[_]] { self => |
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.
Maybe we should make this sealed
, just for future-proofing?
Addresses #3313.