Chinese Locale Support #80

Lhfcws · 2017-03-02T07:46:51Z

Add zh locale support
Add simple url generator in NetworkProducer
fix DateProducerSpec "should generate date between years #fromYear - #toYear", unpass test because the expect result is not correct

2. Add simple url generator in NetworkProducer

…#toYear" unpass because the expect result is not correct

coveralls · 2017-03-02T07:49:43Z

Changes Unknown when pulling 5a630c2 on Lhfcws:master into ** on Codearte:master**.

Lhfcws · 2017-03-02T07:55:11Z

Related issue #78

OlgaMaciaszek

@Lhfcws Thank you very much for your contribution. It looks all right, but there are some changes that should be added. Please have a look at the comments.

OlgaMaciaszek · 2017-03-09T11:56:17Z

src/main/java/io/codearte/jfairy/Bootstrap.java

@@ -115,7 +115,18 @@ public static Fairy create(Provider<DataMaster> dataMaster, Locale locale) {
 	}

 	private static FairyModule getFairyModuleForLocale(DataMaster dataMaster, Locale locale, Random random) {
-		LanguageCode code = LanguageCode.valueOf(locale.getLanguage().toUpperCase());
+		/**


I think the comment is unnecessary. You can add yourself in the file authors' list if you like.
+1 for the functionality.

OlgaMaciaszek · 2017-03-09T12:05:52Z

src/main/java/io/codearte/jfairy/producer/company/CompanyProvider.java

@@ -67,8 +68,27 @@ private void generateDomain() {
 		if (domain != null) {
 			return;
 		}
-		domain = TextUtils.stripAccents(StringUtils.strip(StringUtils.deleteWhitespace(name.toLowerCase()), ".").replace("/", ""))
-				+ "." + dataMaster.getRandomValue(DOMAIN);
+//		domain = TextUtils.stripAccents(StringUtils.strip(StringUtils.deleteWhitespace(name.toLowerCase()), ".").replace("/", ""))


I think it would be actually better to switch CompanyProvider to an interface and add the phonetic conversion. Would you like to provide the conversion mechanism? If you wouldn't like to work on it right now, we could go with a default mechanism. Please let me know. In any case, please remove the commented out code. If we are going to leave the default mechanism, I think it should be simplified, what about just verifying if it has non-latin characters and if yes, generating a random domain?

I've considered the CompanyProvider problem. I just have no idea that changing your default mechanism is proper or not. And if CompanyProvider is changed to Interface, maybe the other providers like PersonProvider should also be changed to keep a uniform design.

I can work on it in 2 or 3 days.

And I think it is better to keep generating a fixed url for each different company if possible. Maybe some people generate pair of company and url relying on this mechanism, but it may not be a big deal.

Maybe migration to Java 8 and a default method in that interface would be useful to reduce amount of changes in other providers?

Yep, this feature is a little bit like Scala trait. It's convenient for code design. But I wonder if there may be a compatibility problem in different Java versions. There're some features in Java8 that cannot be supported in JRE-7, and I'm not sure if this one is included.

It works only with Java 8. However, 2 years ago we started a discussion (#23) to drop support for Java 7 and it seems to be a good argument to do that.

@OlgaMaciaszek @mariuszs Any objections?

0.0 I've already commited Java7 Interface version.

OlgaMaciaszek · 2017-03-09T12:06:48Z

...in/java/io/codearte/jfairy/producer/company/locale/zh/ZhVATIdentificationNumberProvider.java

+			return (char)(65 + r - 10);
+	}
+
+	private String getChars(int padding) {


It seems to be rather the number of chars to return than a padding. Consider renaming the parameter.

OlgaMaciaszek · 2017-03-09T12:15:27Z

src/main/java/io/codearte/jfairy/producer/person/locale/zh/ZhAddress.java

@@ -0,0 +1,70 @@
+package io.codearte.jfairy.producer.person.locale.zh;


Could you extract the Chinese characters to constants?

OlgaMaciaszek · 2017-03-09T13:42:21Z

...in/java/io/codearte/jfairy/producer/company/locale/zh/ZhVATIdentificationNumberProvider.java

+	/**
+	 * Total length of Chinese ID
+	 */
+	private static final int ID_LENGTH = 15;


ID_LENGTH is never used. Can this be removed?

OlgaMaciaszek · 2017-03-09T13:43:46Z

...in/java/io/codearte/jfairy/producer/company/locale/zh/ZhVATIdentificationNumberProvider.java

@@ -0,0 +1,105 @@
+package io.codearte.jfairy.producer.company.locale.zh;


Please change your local variables such as s, r, etc., to have more miningful names.

OlgaMaciaszek · 2017-03-09T13:44:14Z

src/main/java/io/codearte/jfairy/producer/net/NetworkProducer.java

+	 * Add a simple url generator
+	 * @return
+	 */
+	public String url() {


I am not sure if it should be added here. Do you have a usecase for this? Maybe we could just add a method in the CompanyProvider that will use the domain field to generate urls just by prefixing it with https://?

In some cases I need to generate random url for each user, and the original company urls is not enough. I assumed this is a very common demand and I couldn't find a generator in JFairy except company url. And as what I mentioned above, I believe that each company with a fixed url is better.

OlgaMaciaszek · 2017-03-09T13:49:02Z

.../java/io/codearte/jfairy/producer/person/locale/zh/ZhNationalIdentityCardNumberProvider.java

+	/**
+	 * Codes of China provinces
+	 */
+	private static final String[] PROV_LIST = {


Is this the same list that is already used in ZhVATIdentificationNumberProvider? If yes, please extract it somewhere and share between these classes.

OlgaMaciaszek · 2017-03-09T14:05:53Z

.../java/io/codearte/jfairy/producer/person/locale/zh/ZhNationalIdentityCardNumberProvider.java

+	/**
+	 * Codes of China provinces
+	 */
+	private static final String[] PROV_LIST = {


Please change your local variables such as s, r, etc., to have more miningful names.

OlgaMaciaszek · 2017-03-09T14:07:54Z

.../java/io/codearte/jfairy/producer/person/locale/zh/ZhNationalIdentityCardNumberProvider.java

+	/**
+	 * Total length of Chinese ID
+	 */
+	private static final int ID_LENGTH = 18;


ID_LENGTH is never used. Can this be removed?

2. add https option to NetworkProducer.url(boolean isHttps) 3. move common shared constants of zh locale to ZhFairyUtil 4. move comments in Bootstrap.getFairyModuleForLocale to outside as a function's annotations

…ncrete class into DefaultXXProvider 7. FairyModule .configure add implement for guice install-build 8. Fix some test cases after the interface modification 9. Zh locale use ZhCompanyProvider independently

Lhfcws · 2017-03-10T09:15:00Z

@OlgaMaciaszek I've made some modification according to your reviews, including switching the providers of Person/Company/IBAN into Interfaces.

OlgaMaciaszek · 2017-03-15T21:15:47Z

@Lhfcws thank you very much. I will review your new changes tomorrow. The reason we are hesitant when it comes to switching to Java 8 (although we do miss the features a lot, me at least) is that we have some Android users and it seems the support for Java 8 might not yet be stable. We will definitely make a switch once we see no problems for Android.

Lhfcws · 2017-03-27T07:32:59Z

Hi @OlgaMaciaszek . What's the progress of this PR now?

OlgaMaciaszek

Thanks @Lhfcws . It looks really good now :). There just a few very minor changes to add and we will be able to merge. Please have a look at the comments.

OlgaMaciaszek · 2017-04-10T08:12:07Z

src/main/java/io/codearte/jfairy/producer/company/locale/zh/ZhCompanyProvider.java

+	@Override
+	public void generateDomain() {
+		if (domain != null) {
+			return;


As this does not affect the latin languages, I think, it should be moved to CompanyProvider.java as the default implementation. Like this, it will be possible to keep the original Company provider, call super.configure() from ZhFairyModule and keep the FairyModule fields private.

OlgaMaciaszek · 2017-04-10T08:12:48Z

...in/java/io/codearte/jfairy/producer/company/locale/zh/ZhVATIdentificationNumberProvider.java

+
+	private char getChar() {
+		int rndNum = baseProducer.randomBetween(0, 35);
+		if (rndNum < 10)


Plase add {} to improve readability.

OlgaMaciaszek · 2017-04-10T08:13:27Z

src/main/java/io/codearte/jfairy/producer/net/NetworkProducer.java

+			domainChars[i] = (char) (c + 97);
+		}
+
+		String domain = String.valueOf(domainChars);


Please add {} to improve readability.

OlgaMaciaszek · 2017-04-10T08:17:49Z

src/main/java/io/codearte/jfairy/producer/util/ZhFairyUtil.java

+ */
+public class ZhFairyUtil {
+
+	/**


You can add a private default constructor to make sure this class is only used to call static methods.

2. call super.configure() from ZhFairyModule and keep the FairyModule fields private.

coveralls · 2017-04-10T09:41:44Z

Changes Unknown when pulling 291c793 on Lhfcws:master into ** on Codearte:master**.

Lhfcws · 2017-04-10T09:44:10Z

Got it. Done.

lhfcws added 2 commits March 2, 2017 15:20

1. Add zh locale support

a553c55

2. Add simple url generator in NetworkProducer

fix DateProducerSpec "should generate date between years #fromYear - …

5a630c2

…#toYear" unpass because the expect result is not correct

OlgaMaciaszek suggested changes Mar 9, 2017

View reviewed changes

lhfcws added 4 commits March 10, 2017 10:31

1. fix some parameter names issue

4bfadf2

2. add https option to NetworkProducer.url(boolean isHttps) 3. move common shared constants of zh locale to ZhFairyUtil 4. move comments in Bootstrap.getFairyModuleForLocale to outside as a function's annotations

5. add more chinese words in jfairy-zh.yml text field

c95dcb0

6. make Company/Person/IBAN provider into Interface and change the co…

e0e50d0

…ncrete class into DefaultXXProvider 7. FairyModule .configure add implement for guice install-build 8. Fix some test cases after the interface modification 9. Zh locale use ZhCompanyProvider independently

10. make ZhAddress Chinese characters into constants

5926e0f

OlgaMaciaszek reviewed Apr 10, 2017

View reviewed changes

lhfcws added 3 commits April 10, 2017 17:06

add brackets {} to improve readability.

65243b8

1. move non-latin generateDomain() back to default CompanyProvider

7a953b2

2. call super.configure() from ZhFairyModule and keep the FairyModule fields private.

complete some documents

291c793

OlgaMaciaszek merged commit 50e6a55 into Devskiller:master Apr 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chinese Locale Support #80

Chinese Locale Support #80

Lhfcws commented Mar 2, 2017

coveralls commented Mar 2, 2017

Lhfcws commented Mar 2, 2017

OlgaMaciaszek left a comment

OlgaMaciaszek Mar 9, 2017

OlgaMaciaszek Mar 9, 2017

Lhfcws Mar 9, 2017

szpak Mar 9, 2017

Lhfcws Mar 10, 2017 •

edited

Loading

szpak Mar 10, 2017

szpak Mar 10, 2017

Lhfcws Mar 10, 2017

OlgaMaciaszek Mar 9, 2017

OlgaMaciaszek Mar 9, 2017

OlgaMaciaszek Mar 9, 2017

OlgaMaciaszek Mar 9, 2017

OlgaMaciaszek Mar 9, 2017

Lhfcws Mar 9, 2017

OlgaMaciaszek Mar 9, 2017

OlgaMaciaszek Mar 9, 2017

OlgaMaciaszek Mar 9, 2017

Lhfcws commented Mar 10, 2017

OlgaMaciaszek commented Mar 15, 2017

Lhfcws commented Mar 27, 2017

OlgaMaciaszek left a comment

OlgaMaciaszek Apr 10, 2017

OlgaMaciaszek Apr 10, 2017

OlgaMaciaszek Apr 10, 2017

OlgaMaciaszek Apr 10, 2017

coveralls commented Apr 10, 2017

Lhfcws commented Apr 10, 2017

		@@ -0,0 +1,70 @@
		package io.codearte.jfairy.producer.person.locale.zh;

		@@ -0,0 +1,105 @@
		package io.codearte.jfairy.producer.company.locale.zh;

Chinese Locale Support #80

Chinese Locale Support #80

Conversation

Lhfcws commented Mar 2, 2017

coveralls commented Mar 2, 2017

Lhfcws commented Mar 2, 2017

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lhfcws Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lhfcws commented Mar 10, 2017

OlgaMaciaszek commented Mar 15, 2017

Lhfcws commented Mar 27, 2017

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 10, 2017

Lhfcws commented Apr 10, 2017

Lhfcws Mar 10, 2017 •

edited

Loading