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

Feature/add jvm target #159

Merged
merged 17 commits into from
Aug 24, 2021
Merged

Conversation

nrobi144
Copy link
Contributor

There are a couple of open-questions, created a draft PR to hopefully answer them

@nrobi144
Copy link
Contributor Author

Will check-out the detekt issues & pipeline messages a bit later


actual class ResourceFormattedStringDesc actual constructor(
private val stringRes: StringResource,
private val args: List<Any>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now all the android formatted strings we'll be replaced with {n} when we generate the StringResource, this means that we lose the type of the argument.

One possible solution would be to leave them as they are, and replace them prior to formatting (and get the necessary type information, before localizing them). See MokoBundle

@@ -33,7 +32,10 @@ object Testing {
MR.plurals.test_plural.desc(1),
MR.plurals.test_plural.desc(2),
MR.plurals.test_plural.desc(3),
nestedTest()
MR.plurals.test_plural.desc(7),
// TODO nested won't work with the current implementation, since the library tries to resolve from a bundle
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how could we solve this

Copy link
Member

Choose a reason for hiding this comment

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

plugin should collect all nested resources too. just like in apple generator implemented

Copy link
Contributor Author

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 know what you mean, currently resolving the nested will cause a

Can't find resource for bundle java.util.PropertyResourceBundle, key nested.test

I think on android this works by default, because how android aggregates or searches through the sources, but I may be wrong.
I didn't research much on this, but maybe there is a similar solution for this on the jvm side too.
Did you meant copying over the generated resources from the nested modules, I'd consider that a bit overkill, but I'm not sure

Copy link
Member

Choose a reason for hiding this comment

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

in uber-jar all resources aggregates into single jar, so if we have same name of some resources (like MokoBundle for localizations) we got only one version of this files. to fix it i will move generation of files into package-dependend path

@Alex009 Alex009 linked an issue Jan 23, 2021 that may be closed by this pull request
buildSrc/build.gradle.kts Outdated Show resolved Hide resolved
@Alex009
Copy link
Member

Alex009 commented Jan 23, 2021

also please fix detekt problems, CI failed because of them

@@ -33,7 +32,10 @@ object Testing {
MR.plurals.test_plural.desc(1),
MR.plurals.test_plural.desc(2),
MR.plurals.test_plural.desc(3),
nestedTest()
MR.plurals.test_plural.desc(7),
// TODO nested won't work with the current implementation, since the library tries to resolve from a bundle
Copy link
Member

Choose a reason for hiding this comment

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

plugin should collect all nested resources too. just like in apple generator implemented

settings.gradle.kts Outdated Show resolved Hide resolved
@Alex009
Copy link
Member

Alex009 commented Jan 23, 2021

also i got error when try to compile sample jvm:

17:38:36: Executing task 'run'...

Executing tasks: [run] in project /Users/alekseymikhailovwork/Documents/development/icerockdev_workspace/moko/moko-resources/sample/desktop-app

> Task :plugins:buildSrc:compileKotlin UP-TO-DATE
> Task :plugins:buildSrc:compileJava NO-SOURCE
> Task :plugins:buildSrc:compileGroovy NO-SOURCE
> Task :plugins:buildSrc:processResources NO-SOURCE
> Task :plugins:buildSrc:classes UP-TO-DATE
> Task :plugins:buildSrc:inspectClassesForKotlinIC UP-TO-DATE
> Task :plugins:buildSrc:jar UP-TO-DATE
> Task :plugins:buildSrc:assemble UP-TO-DATE
> Task :plugins:buildSrc:compileTestKotlin NO-SOURCE
> Task :plugins:buildSrc:compileTestJava NO-SOURCE
> Task :plugins:buildSrc:compileTestGroovy NO-SOURCE
> Task :plugins:buildSrc:processTestResources NO-SOURCE
> Task :plugins:buildSrc:testClasses UP-TO-DATE
> Task :plugins:buildSrc:test NO-SOURCE
> Task :plugins:buildSrc:check UP-TO-DATE
> Task :plugins:buildSrc:build UP-TO-DATE
> Task :buildSrc:compileKotlin UP-TO-DATE
> Task :buildSrc:compileJava NO-SOURCE
> Task :buildSrc:compileGroovy NO-SOURCE
> Task :buildSrc:processResources NO-SOURCE
> Task :buildSrc:classes UP-TO-DATE
> Task :buildSrc:inspectClassesForKotlinIC UP-TO-DATE
> Task :buildSrc:jar UP-TO-DATE
> Task :buildSrc:assemble UP-TO-DATE
> Task :buildSrc:compileTestKotlin NO-SOURCE
> Task :buildSrc:compileTestJava NO-SOURCE
> Task :buildSrc:compileTestGroovy NO-SOURCE
> Task :buildSrc:processTestResources NO-SOURCE
> Task :buildSrc:testClasses UP-TO-DATE
> Task :buildSrc:test NO-SOURCE
> Task :buildSrc:check UP-TO-DATE
> Task :buildSrc:build UP-TO-DATE
> Task :plugins:resources-generator:compileKotlin UP-TO-DATE
> Task :plugins:resources-generator:compileJava NO-SOURCE
> Task :plugins:resources-generator:pluginDescriptors UP-TO-DATE
> Task :plugins:resources-generator:processResources UP-TO-DATE
> Task :plugins:resources-generator:classes UP-TO-DATE
> Task :plugins:resources-generator:inspectClassesForKotlinIC UP-TO-DATE
> Task :plugins:resources-generator:jar UP-TO-DATE

> Configure project :resources
used new ios() shortcut target

> Configure project :sample:mpp-library
used new ios() shortcut target

> Configure project :sample:mpp-library:nested-module
used new ios() shortcut target

> Task :sample:desktop-app:jvmProcessResources NO-SOURCE
> Task :sample:mpp-library:nested-module:jvmProcessResources
> Task :sample:mpp-library:jvmProcessResources
> Task :resources:compileKotlinJvm
> Task :resources:jvmProcessResources NO-SOURCE
> Task :resources:jvmMainClasses
> Task :resources:jvmJar

> Task :sample:mpp-library:nested-module:compileKotlinJvm FAILED

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.8/userguide/command_line_interface.html#sec:command_line_warnings
5 actionable tasks: 5 executed
e: /Users/alekseymikhailovwork/Documents/development/icerockdev_workspace/moko/moko-resources/sample/mpp-library/nested-module/build/generated/moko/commonMain/src/com/icerockdev/library/nested/MR.kt: (11, 15): Expected object 'MR' has no actual declaration in module <nested-module> for JVM

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':sample:mpp-library:nested-module:compileKotlinJvm'.
> Compilation error. See log for more details

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 12s
17:38:49: Task execution finished 'run'.

@Alex009 Alex009 added this to the 0.15.0 milestone Jan 23, 2021
@Alex009
Copy link
Member

Alex009 commented Mar 9, 2021

i add some changes in own branch https://github.com/icerockdev/moko-resources/tree/%23151-jvm-target-support
now branch contain not completed support of multimodule and some fixes for building of all samples

@Alex009
Copy link
Member

Alex009 commented Mar 9, 2021

here current changes between branches 3e8012a...#151-jvm-target-support

@Alex009 Alex009 self-assigned this Mar 9, 2021
@nrobi144
Copy link
Contributor Author

@Alex009 probably we should migrate to PluralRules instead of ChoiceFormat, see https://stackoverflow.com/a/14327683/4487858 for more info

@Alex009 Alex009 changed the base branch from master to develop March 11, 2021 12:39
@nrobi144
Copy link
Contributor Author

@Alex009 in 97489b6 I've migrated to PluralRules implementation.
Regarding

<string name="positional">second string %2$s first decimal %1$d</string>

The current jvm implementation requires that args should be passed in correct order, since we replace every argument with {0}, {1}, ... {n} only.
If we want to cover this aspect also, we have two options:

  1. Create a proper mapper where %1$d -> {0, number, integer}
  2. Using a different formatter then MessageFormat may fix the issue

I think 2) would be a far better option

@Alex009 Alex009 linked an issue Apr 8, 2021 that may be closed by this pull request
@Alex009 Alex009 modified the milestones: 0.16.0, 0.17.0 Jun 25, 2021
@anton6tak anton6tak merged commit 9c7c6ab into icerockdev:develop Aug 24, 2021
@hmqgg
Copy link

hmqgg commented Aug 24, 2021

Any ETA to publish a release with this PR?

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.

Using common fonts in Jetpack Compose Add JVM target
4 participants