Skip to content

Bindy unicode patch#3552

Merged
davsclaus merged 4 commits intoapache:masterfrom
mgr-lhm:bindy-unicode-patch
Feb 11, 2020
Merged

Bindy unicode patch#3552
davsclaus merged 4 commits intoapache:masterfrom
mgr-lhm:bindy-unicode-patch

Conversation

@mgr-lhm
Copy link
Copy Markdown
Contributor

@mgr-lhm mgr-lhm commented Feb 7, 2020

Copy link
Copy Markdown
Contributor

@oscerd oscerd left a comment

Choose a reason for hiding this comment

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

I think this is a good improvement, but this requires adding a license notice for icu4j from what I'm seeing in other ASF projects. @davsclaus @gnodet what do you think?

import com.ibm.icu.text.BreakIterator;

/**
* <p>Unterstützung für String-Verarbeitung für Unicode-Strings</p>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

</dependency>
<dependency>
<groupId>com.ibm.icu</groupId>
<artifactId>icu4j</artifactId>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using this will require adding a license for this.

https://github.com/search?q=org%3Aapache+%27com.ibm.icu.icu4j%27&type=Code

Other projects are already using it, not sure this is a good idea, by the way.

Copy link
Copy Markdown
Member

@omarsmak omarsmak left a comment

Choose a reason for hiding this comment

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

Can you please change the comments from German to English :).
Also can you please run the build with enabled checkstyle mvn -Psourcecheck clean install, there are couple of violations need to be fixed

<description>Camel Bindy data format support</description>

<properties>
<icu4j.version>65.1</icu4j.version>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please put the version property here as it is easier to manage

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

* Made UnicodeHelper Serializble and added toString().
@oscerd
Copy link
Copy Markdown
Contributor

oscerd commented Feb 10, 2020

@davsclaus about the icu4j dependency do we want to add a license file for it like Hive is doing?

@davsclaus
Copy link
Copy Markdown
Contributor

Yeah there are a number of other Apache projects use it as listed on this page (who is using)
http://site.icu-project.org/

@mgr-lhm
Copy link
Copy Markdown
Contributor Author

mgr-lhm commented Feb 10, 2020

I adjusted the pull request as suggested:

  • No more sourcecheck errors any more.
  • Comments and JavaDoc are now english.
  • Version of icu dependency is no in camel-parent pom.

@davsclaus
Copy link
Copy Markdown
Contributor

@davsclaus
Copy link
Copy Markdown
Contributor

And configuring that option to choose how to count (eg that enum) should be added as an option to bindy data format in core (its a bit complex to do). But we can do that in a 2nd PR.

@omarsmak omarsmak self-requested a review February 10, 2020 09:24
@davsclaus davsclaus merged commit 0e36d91 into apache:master Feb 11, 2020
@davsclaus
Copy link
Copy Markdown
Contributor

Ah the count graphme option is configured on the model annotations, then this is fine.

@mgr-lhm
Copy link
Copy Markdown
Contributor Author

mgr-lhm commented Feb 12, 2020

It was a pleasure to work with the Camel Community :-)

@davsclaus: I have one more question on this topic: In which versions of camel will this improvement be available? Will it be back-ported to older versions and if yes to which?

@davsclaus
Copy link
Copy Markdown
Contributor

Camel 3.1, and no it wont be backported.

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.

4 participants