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

Added lazy mountable extensions #3187

Merged
merged 4 commits into from Oct 2, 2022
Merged

Added lazy mountable extensions #3187

merged 4 commits into from Oct 2, 2022

Conversation

sksamuel
Copy link
Member

Would something as simple as this suffice for your needs @nomisRev

@sksamuel sksamuel requested a review from nomisRev August 28, 2022 19:44
Copy link
Contributor

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

I'm so sorry @sksamuel, my comment has been pending on this PR for 2 + weeks.. 🤦‍♂️

this.value = value
}

fun get() = value
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not be protected from incorrect access during initialisation? It can only be safely accessed from TestScope? That actually depends on how the underlying Extension initialises it, right?

This would work for my use-case, but the API is rather unsafe.

In the PR with the original use-case here there is LazyMaterialized with suspend fun get() so it can only be accessed from suspend TestScope.() -> Unit. If it's access before it needs to be somehow in a suspend context and will get loaded lazily then. If it's accessed after clear/close is called (on afterSpec or similar) then it'll throw an IllegalStateException.

Should we have a similarly "safer" API? I.e. the example here Kafka also involves some heavy I/O, blocking code both on start and close.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, short-answer: yes this works.
Long-answer: should we try to make the API safer?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree the API isn't great, but I don't know how we can limit it to test scope, or even if we care about that, as long as it supports suspension.
Like you might want to initialize in a before spec listener. That's a suspendable context.
Let me think more on this.

Copy link
Member Author

@sksamuel sksamuel Oct 2, 2022

Choose a reason for hiding this comment

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

Ok I have followed your guidelines and it is much clearer now.

A LazyMountableExtension returns a LazyMaterialized which has a SAM.

interface LazyMaterialized<MATERIALIZED> {
   suspend fun get(): MATERIALIZED
}

Once get is invoked, the implementation can initialize if required.

Here is the example from the tests:

object : LazyMaterialized<String> {
  var state: String? = null
  override suspend fun get(): String {
    delay(1)
    if (state == null) state = "ready"
    return state ?: error("Must be initialized")
  }
}

@sksamuel sksamuel enabled auto-merge (squash) October 2, 2022 22:37
@sksamuel sksamuel merged commit 0ad2a8a into master Oct 2, 2022
@sksamuel sksamuel deleted the feature/lazy_mounts branch October 2, 2022 23:10
@nomisRev
Copy link
Contributor

nomisRev commented Oct 3, 2022

Thank you for looking into this @sksamuel 🙏
This is going to be great to offer even better support for Arrow in Kotest auto-of-the-box ❤️

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.

None yet

3 participants