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

Add i18n module #3

Merged
merged 6 commits into from
Jun 21, 2021
Merged

Add i18n module #3

merged 6 commits into from
Jun 21, 2021

Conversation

chimp1984
Copy link
Contributor

No description provided.

I am aware that buildSrc is more for common tasks instead of sharing dependencies, but as we use it for logging as well I think it is convenient to repeat that pattern for application wide used dependencies. At least I don't see any downside...

After some doubts I tend to be in favor of adding lombok as it improves productivity and reduces boilerplate code and thus increase readability.
Java records are not flexible enough to replace all use cases for value objects, as well as other annotations are convenient as well.
This will hold translation strings. It is intended to use different files for different aspects of the application. If we can match the module boundaries is questionable. I assume that might be too fine grained.
That module has no dependency to UI so can be used also for error logging in headless context, etc.
@chimp1984 chimp1984 mentioned this pull request Jun 12, 2021
chimp1984 and others added 2 commits June 12, 2021 13:38
This changes a bit the behaviour in case of null values. We throw an IOException now, before it was returning null
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Open question about whether git whitespace warnings are acceptable.
Other than that minor issue, ACK.

@chimp1984
Copy link
Contributor Author

Open question about whether git whitespace warnings are acceptable.

Where do you get that?

There are no lombok dependency version contraints defined in common-platform,
so lombok-dependencies.gradle does not need to consume that platform.

In the normal use case, lombok dependency version contraints would be defined
in common-platform, and common-platform would be consumed by
lombok-dependencies.gradle.  However, this does not work for the gradle
dependency configuration types `annotationProcessor` and `testAnnotationProcessor`
(could be a Gradle bug).  In this corner case, we do have to specify the version
as @chimp1984 has done in lombok-dependencies.gradle.
@ghubstan
Copy link
Contributor

Open question about whether git whitespace warnings are acceptable.

Where do you get that?

It is odd, new git diff behaviour. If I add a newline to the end of a file, git complains:

$ git diff buildSrc/lombok-dependencies.gradle
diff --git a/buildSrc/lombok-dependencies.gradle b/buildSrc/lombok-dependencies.gradle
index cac2e6a..e968399 100644
--- a/buildSrc/lombok-dependencies.gradle
+++ b/buildSrc/lombok-dependencies.gradle
@@ -5,4 +5,4 @@ dependencies {
     annotationProcessor 'org.projectlombok:lombok:1.18.20'
     testCompileOnly 'org.projectlombok:lombok:1.18.20'
     testAnnotationProcessor 'org.projectlombok:lombok:1.18.20'
-}
\ No newline at end of file
+}

We only need logback.xml files in application level modules, not
every module.  And logback complains about finding multiple
logback.xml files in the classpath.
@@ -0,0 +1,7 @@
dependencies {
api platform(project(':platforms:common-platform'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Commit 6edd97c removes api platform(project(':platforms:common-platform')) as per the commit's comment.

@@ -0,0 +1,13 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Commit 5db7a1a removes i18n/src/main/resources/logback.xml. See the commit's comment.

@ghubstan
Copy link
Contributor

I made two changes (6edd97c, 5db7a1a) I cannot review myself. This PR can be merged if @jmacxx reivews and ACKs it again.

@ghost ghost self-requested a review June 21, 2021 15:31
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

utACK

@ghubstan
Copy link
Contributor

ACK

@ghubstan ghubstan merged commit ecd2621 into bisq-network:main Jun 21, 2021
@chimp1984 chimp1984 deleted the add_i18n_module branch November 18, 2021 00:58
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