Add common module #4
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
This is a collection of used utils and common classes from the misq prototype. It has only few tests so far. Many classes are copied or derived from Bisq. There might be some tests for those to take over. The monetary package was created in favor over the BitcoinJ monetary classes as those have heavily overloaded APIs and do not cover the use cases we need. They are centered around BTC but we need to support fiat-fiat as well as altcoin-altcoin pairs. Beside that the exchange rate API was very un-intuitive to use and did not cover all use cases. The Quote class should handle all the use cases we need and was used already in the prototype project for the offer book domain. Handling of BigInteger and BigDecimal might need more tests and checks if it handles overflow scenarios correctly.
chimp1984
force-pushed
the
add_common_module
branch
from
3a95e85 to
fdf1d93
Compare
Co-authored-by: Kai <[email protected]>
This changes a bit the behaviour in case of null values. We throw an IOException now, before it was returning null
Merged
kaiwolfram
reviewed
Jun 12, 2021
common/src/main/java/network/misq/common/currency/FiatCurrency.java
Outdated
Show resolved
Hide resolved
common/src/main/java/network/misq/common/currency/MisqCurrency.java
Outdated
Show resolved
Hide resolved
common/src/main/java/network/misq/common/locale/LocaleRepository.java
Outdated
Show resolved
Hide resolved
common/src/main/java/network/misq/common/util/DecimalFormatters.java
Outdated
Show resolved
Hide resolved
common/src/main/java/network/misq/common/util/StatisticsUtils.java
Outdated
Show resolved
Hide resolved
….java Co-authored-by: Kai <[email protected]>
….java Co-authored-by: Kai <[email protected]>
…ry.java Co-authored-by: Kai <[email protected]>
…ry.java Co-authored-by: Kai <[email protected]>
Co-authored-by: Kai <[email protected]>
Co-authored-by: Kai <[email protected]>
Co-authored-by: Kai <[email protected]>
…does not justify to add a new dependency.
|
@KaiWitt Great thanks for the reviews! |
ghost
suggested changes
Jun 14, 2021
There was a problem hiding this comment.
Other than the one build error, there were several complaints from git about trailing spaces.
../misqpull4.patch:480: trailing whitespace.
../misqpull4.patch:788: trailing whitespace.
// We only can check if the currency is not fiat and if the code matches the format, but we do not maintain a list
../misqpull4.patch:789: trailing whitespace.
// of crypto currencies to be flexible with any newly added one.
../misqpull4.patch:3868: trailing whitespace.
* We only can check if the currency is not fiat and if the code matches the format, but we do not maintain a list
../misqpull4.patch:239: new blank line at EOF.
+
warning: 4 lines applied after fixing whitespace errors.
How did you get those complaints? |
|
The whitespace errors are produced when applying the PR locally using We should set up some automatic build tools like the Bisq repo has, perhaps can discuss at the status meeting. |
Yes definitely! Should have been done initially, forgot about it... |
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.
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.
Non-app level modules do not need their own logback config, but some module's tests might. (Test logback.xml files will be excluded from shipped jars, but src/logback.xml files would be included, and make logback complain about multiple config files in the runtime classpath.)
A decision was made to use lombok.
ghost
self-requested a review
|
ACK |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on #3
I did only add few tests so far.
If any developer wants to contribute to add more unit tests to that module would be great. I need to focus to get progress on other areas...Some tests can be taken over from Bisq as well.