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

Create Temporal Module #33

Merged
merged 10 commits into from
Jan 28, 2021
Merged

Create Temporal Module #33

merged 10 commits into from
Jan 28, 2021

Conversation

cedrickcooke
Copy link
Contributor

  • Adds broadcastReceiverFlow for Android in :coroutines module
  • Adds a series of helper assertions to :test module.
  • Creates new :temporal module
    • Adds an internal ticker flow which allows time based ticking. Implementation is similar to Kotlin's ticker channel, but a little less robust/more simple.
    • Exposes public flows for time changes.

@cedrickcooke cedrickcooke requested a review from twyatt January 26, 2021 00:12
@cedrickcooke cedrickcooke marked this pull request as ready for review January 26, 2021 00:14
@cedrickcooke cedrickcooke requested review from a team and sdonn3 January 26, 2021 00:14
android.useAndroidX=true

# Android Build Features
android.defaults.buildfeatures.buildconfig=false
Copy link
Member

Choose a reason for hiding this comment

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

TIL

I always did this in the build.gradle.kts's android block. Cool to know you can just blanket disable in a project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know (could be wrong though), it's relatively new. These change defaults for the "build features" block, which I am pretty sure was only added with view binding.

temporal/README.md Outdated Show resolved Hide resolved
temporal/README.md Outdated Show resolved Hide resolved
temporal/src/androidMain/AndroidManifest.xml Outdated Show resolved Hide resolved
temporal/src/commonMain/kotlin/TemporalFlow.kt Outdated Show resolved Hide resolved
build.gradle.kts Outdated Show resolved Hide resolved
build.gradle.kts Outdated Show resolved Hide resolved
temporal/api/temporal.api Outdated Show resolved Hide resolved
Comment on lines +11 to +13
public static final fun assertSimilar-2yBKrUY (Lkotlinx/datetime/LocalDateTime;DLkotlinx/datetime/LocalDateTime;Lkotlinx/datetime/TimeZone;)V
public static synthetic fun assertSimilar-2yBKrUY$default (Lkotlinx/datetime/LocalDateTime;DLkotlinx/datetime/LocalDateTime;Lkotlinx/datetime/TimeZone;ILjava/lang/Object;)V
public static final fun assertSimilar-hN_NL7I (Lkotlinx/datetime/Instant;DLkotlinx/datetime/Instant;)V
Copy link
Member

Choose a reason for hiding this comment

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

Are you aware of a ware to prevent name mangling on our public API? Or is this by design, as they're internal so shouldn't be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These functions are properly public, not @PublishedApi internal. The mangling here is because the function accepts a kotlin.time.Duration argument (which is an inline class wrapper around a Double).

@twyatt twyatt self-requested a review January 26, 2021 20:33
Copy link
Member

@twyatt twyatt left a comment

Choose a reason for hiding this comment

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

Overall looks great. Just had some probably quick concerns to address.

crossinline factory: () -> T
): Flow<T> = flow {
emit(factory.invoke())
emitAll(broadcastReceiverFlow(temporalEventFilter).map { factory.invoke() })
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to output which Filter generated the event, as a parameter in the lambda?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wouldn't be hard to add an Intent argument to the factory lambda, but doing that would require the API not to match other platforms.

Copy link
Member

@twyatt twyatt left a comment

Choose a reason for hiding this comment

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

Love all the tests.

It's great to see that one of the benefits of breaking this component out of the app layer and into an external library is that we get the luxury of allowing tests to have short delays. In the app layer I'd be worried we'd be extending the duration of our already long test suite, but this library isn't build as frequently and overall its test suite is nice and short. Adding these longer running tests isn't a big deal at all.

Comment on lines +43 to +44
// If this is an application module and your minimum API version is below 26, enable core library
// desugaring. See https://developer.android.com/studio/write/java8-support#library-desugaring
Copy link
Member

Choose a reason for hiding this comment

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

❤️
Super helpful documentation.

@cedrickcooke cedrickcooke merged commit bca8844 into main Jan 28, 2021
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.

3 participants