Handle date/time conversion in runtime via instantiable `FormattedResources` #390

drewhamilton · 2024-07-31T18:37:58Z

Implements Patrick's idea from #258. Closes #230.

This pulls all android.icu imports out of generated code and into the runtime, allowing substitution of non-Android ICU dependencies in JVM unit tests. A generic DateTimeConverter interface is introduced to format date/time parameters. The default AndroidDateTimeConverter implementation instantiates android.icu.util.Calendars. JVM tests can substitute a JvmDateTimeConverter into their FormattedResources instance to instantiate com.ibm.icu.util.Calendars, avoiding the need for Robolectric to format date/time strings.

This also makes the generated FormattedResources an instantiable class, rather than an object, so the appropriate DateTimeConverter can be injected. A default AndroidFormattedResources property is introduced so simple static access is still supported.

drewhamilton · 2024-07-31T18:39:10Z

plugin/src/main/java/app/cash/paraphrase/plugin/ResourceWriter.kt

+        .addProperty(
+          PropertySpec.builder(
+            name = "dateTimeConverter",
+            type = Types.DateTimeConverter.parameterizedBy(ANY),


There's not a more concise way to share the ParameterSpec and the PropertySpec that have the same name and type, is there?

drewhamilton · 2024-07-31T18:51:10Z

plugin/src/main/java/app/cash/paraphrase/plugin/ResourceWriter.kt

    .addImport(packageName = packageName, "R")
+    .addProperty(
+      PropertySpec.builder(
+        name = "AndroidFormattedResources",


I went back and forth on how to name these:

BaseFormattedResources and FormattedResources was Patrick's original suggestion, and allows for the simplest transition for current consumers. But it's the base class that most consumers would probably deal with, so giving it the longer name feels clunky. And having many different classes generated with the same prefix Base might be confusing since it is not a common base.

FormattedResources and AndroidFormattedResources is what I've gone with. The generic type has the snappier name. But it makes the transition for current consumers more involved. And using the prefix Android might seem redundant for an Android-targeted library.

Another idea: Rename AndroidFormattedResources to FR. This makes static invocations somewhat parallel to static invocations of the standard Android resource: R.string.whatever → FR.whatever(args). But it's still an involved transition from the previous Paraphrase API. And maybe too cute.

Curious what others think for naming these two.

I don't love the Base prefix either, I'm happy to go with either of the other two options. Another option might be a base of ParaphraseResources and implementations of AndroidResources and JvmResources.

I'm not sure there's a perfect naming scheme and I'm also not sure it's a big deal. I'm cool with just picking something and shipping it!

I like ParaphraseResources because it steps around the verb-tense problem with "Formatted" and also reminds you what this thing is.

That would also let me include a deprecated val FormattedResources with a built-in ReplaceWith to the new name, if we want to get fancy and make the transition easy.

sample/library/src/test/kotlin/app/cash/paraphrase/sample/library/JvmDateTimeConverter.kt

tests/src/main/kotlin/app/cash/paraphrase/tests/JvmDateTimeConverter.kt

theisenp

Nice, seems like this worked out pretty well! Thanks for putting it together 🙏

Now that we have both this and #258, does one approach feel better than the other?

theisenp · 2024-10-10T21:12:46Z

sample/app/src/main/java/app/cash/paraphrase/sample/app/MainActivity.kt

+    private fun appSamples(
+      formattedResources: AppFormattedResources,
+    ): List<Sample> = listOf(


If we wanted to keep this as a val with static references to AppAndroidFormattedResources that would still work right? Are we just switching to a fun to demonstrate that you can pass the resources as the interface type?

Yeah exactly, just demonstrating interaction with the interface type.

theisenp · 2024-10-10T21:23:29Z

sample/library/src/test/kotlin/app/cash/paraphrase/sample/library/FormattedResourcesTest.kt

+  private val stringResolver = FakeStringResolver(
+    R.string.library_date_argument to "{release_date, date, short}",
+    R.string.library_time_argument to "{showtime, time, short}",
+  )


Is the resolver type here useful, or could we just have the Map<Int, String>?

I can switch to a map if you prefer. I think I was just writing what felt like a real app might do; write a fake stand-in for an Android resource-resolving interface.

theisenp · 2024-10-10T21:26:10Z

sample/library/src/test/kotlin/app/cash/paraphrase/sample/library/FormattedResourcesTest.kt

+import java.util.Locale
+import org.junit.Test
+
+class FormattedResourcesTest {


Is the idea here just to show people how they might override the FormattedResources type in tests?

This tests that date/time resolution works in a JVM test. This test fails on main because of the ICU4J/android.icu discrepancy.

theisenp · 2024-10-10T21:43:25Z

plugin/src/main/java/app/cash/paraphrase/plugin/ResourceWriter.kt

    .addImport(packageName = packageName, "R")
+    .addProperty(
+      PropertySpec.builder(
+        name = "AndroidFormattedResources",


I don't love the Base prefix either, I'm happy to go with either of the other two options. Another option might be a base of ParaphraseResources and implementations of AndroidResources and JvmResources.

I'm not sure there's a perfect naming scheme and I'm also not sure it's a big deal. I'm cool with just picking something and shipping it!

drewhamilton · 2024-10-17T20:39:40Z

Now that we have both this and #258, does one approach feel better than the other?

This one feels better. We've all been through the swapping-static-references-in-tests phase before and agree it's painful, so let's not go there.

to handle converting to ICU Calendar

This prevents the ULocale from being instantiated at class-loading time, which would crash JVM tests due to the missing Android implementation.

…attedResources This supports testing with an injected JvmDateTimeFormatter, rather than setting it on a static instance.

…ming

drewhamilton requested review from JakeWharton and theisenp July 31, 2024 18:37

drewhamilton commented Jul 31, 2024

View reviewed changes

drewhamilton force-pushed the drew/handle-date-time-conversion-in-runtime-via-injection branch 3 times, most recently from f4873dc to 0070c19 Compare July 31, 2024 21:20

theisenp approved these changes Oct 10, 2024

View reviewed changes

drewhamilton and others added 16 commits October 17, 2024 15:40

Add DateTimeConverter to runtime

e59ddad

to handle converting to ICU Calendar

Generate DateTimeConverter instance in FormattedResources

1363960

Rename all functions to convertToCalendar

f9a4c49

Generate use of dateTimeConverter in FormattedResources

1e7ea0c

Convert AndroidDateTimeConverter locale to lazy

8d4a7da

This prevents the ULocale from being instantiated at class-loading time, which would crash JVM tests due to the missing Android implementation.

Add sample FormattedResourcesTest using dateTimeConverter substitution

71ed64c

API dump

ad34fc9

Delete extra copys

129e9bf

Add SubclassOptInRequired to DateTimeConverter

128e07c

Test ICU4J in TypesTest to compare results with Android ICU

7c7bd72

Make dateTimeConverter public, visible for testing

4257663

Generate injectable FormattedResources along with default AndroidForm…

2ca2f81

…attedResources This supports testing with an injected JvmDateTimeFormatter, rather than setting it on a static instance.

Fix new NNBSP logic in TypesTest

91f00b7

Rename FormattedResources to ParaphraseResources

d02b811

Add deprecated FormattedResources val to simplify migration to new na…

31c64d0

…ming

Publish JvmDateTimeConverter in new :runtime-test module

12d4335

drewhamilton force-pushed the drew/handle-date-time-conversion-in-runtime-via-injection branch from 0070c19 to 12d4335 Compare October 18, 2024 16:17

Spotless

087cf0d

Handle date/time conversion in runtime via instantiable FormattedResources #390

Are you sure you want to change the base?

Handle date/time conversion in runtime via instantiable FormattedResources #390

Uh oh!

Conversation

drewhamilton commented Jul 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

theisenp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drewhamilton commented Oct 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Handle date/time conversion in runtime via instantiable `FormattedResources` #390

Handle date/time conversion in runtime via instantiable `FormattedResources` #390