Add locale for GE #92
Conversation
There was a problem hiding this comment.
@EduardRindt-4F thanks for this PR. It looks good, but I've added a few comments. Please have a look.
| @@ -147,6 +143,8 @@ private static FairyModule getFairyModuleForLocale(DataMaster dataMaster, Locale | |||
| return new ZhFairyModule(dataMaster, randomGenerator); | |||
| case DE: | |||
| return new DeFairyModule(dataMaster, randomGenerator); | |||
| case KA: | |||
There was a problem hiding this comment.
Is the language code GE or KA?
There was a problem hiding this comment.
As mentioned in the PR description, this a rare case where a name of the country and language differs. 'GE' is the country (ISO 3166) , 'KA' is the language (https://www.loc.gov/standards/iso639-2/php/langcodes_name.php?iso_639_1=ka).
There was a problem hiding this comment.
Ok. Looking back at it, the way it's done right now is inconsistent. We use language codes, while the fairy modules really represent specific locale, not only languages. I think you can keep either KA or GE here, but soon we should modify these enums to represent both language and country, cause there can be different document formats in same language speaking countries. However, either way, I would only stick with one of them for now till we get it fixed. Please choose one and stick to it in all your changes.
There was a problem hiding this comment.
Well, there is not much to be decided. The Georgian locale must have the 'ka' language as per ISO-639. If I don't want to change the language-to-module deduction logic in Bootstrap.getFairyModuleForLocale(...) as well as the parser of control YAMLs, I have to stay with ka.
I do not like it (KaFairyModule etc. is counter-intuitive), but I shall follow your rules.
See my latest commit, please.
| /** | ||
| * An address format typical for European countries but the UK and ex-Soviet union. | ||
| */ | ||
| public abstract class ContinentalAddress extends AbstractAddress { |
There was a problem hiding this comment.
I'm not sure if 2 levels of abstractions are needed here. Maybe it would be better to leave the implementation of the methods that most countries use in AbstractAddress, and just override it in Ge, En and Zh?
There was a problem hiding this comment.
I believe that the order of address parts ( street > number, ZIP > city) is really quite common for European countries, but nowhere else.
What is wrong with one intermediate class shared by four implementations?
| this.baseProducer = baseProducer; | ||
| } | ||
|
|
||
| // District codes are not reachable at the moment. |
There was a problem hiding this comment.
What does this mean that they are not reachable? Is there any list of codes or random numbers are being used?
There was a problem hiding this comment.
I could not find the codes anywhere, GE Ministry of Interior has not published them.
| } | ||
|
|
||
| // A system of Ministry of Inferior Office codes is not known at the moment. | ||
| private String getStateOfficeCode(String district) { |
There was a problem hiding this comment.
What does this mean that they are not known? Is there any list of codes or random numbers are being used?
There was a problem hiding this comment.
The same as for district codes.
| import spock.lang.Specification | ||
|
|
||
| class GeAddressSpec extends Specification { | ||
| private final String NEW_LN = System.lineSeparator(); |
There was a problem hiding this comment.
Please add static import.
| import spock.lang.Specification | ||
|
|
||
| class GeNationalIdentityCardNumberProviderSpec extends Specification { | ||
| private GeNationalIdentityCardNumberProvider generator |
There was a problem hiding this comment.
Maybe better idCardNumberProvider or provider?
| import spock.lang.Specification | ||
|
|
||
| class GePassportNumberProviderSpec extends Specification { | ||
| private GePassportNumberProvider generator |
There was a problem hiding this comment.
Maybe better idCardNumberProvider or provider?
|
I've replied to some objections and added a commit with fixes of the rest. |
|
@EduardRindt-4F I have left you one more comment that requires fixing regarding the language codes. Please have a look. |
EduardRindt-4F
force-pushed
the
locale-ge
branch
from
e74bc93 to
ff93656
Compare
|
A fix is ready for another review iteration. |
|
@EduardRindt-4F thank you for the changes. I do agree it is less intuitive, but it is consistent with the previous contributions. |
|
Cleaned-up & added one more unit test. |
An attempt to extend JFairy by the GE locale.
Note 1: ISO-639 code for Georgian language is "ka". I preferred using the (country code) "ge" wherever possible.
Note 2: I neither speak Georgian nor have any detailed knowledge of the country. The module is based just of information googled from the net.