DatabaseGenerator is concatenating leadingDigits collection from XML source into one pattern string about globalphone HOT 6 CLOSED

That looks indeed like a bug. Perhaps that we should add your regression test?

from globalphone.

Good work on finding how and why of this issue.

from globalphone.

Thanks :). I'm glad to hear that my description was understandable and that we both agree it's a problem.

To help understand the impact, I did a quick XPath query of the current master PhoneNumberMetadata.xml file on GitHub, and noticed that 22 out of 253 territories have at least one number format definition with more than one leadingDigits element. That means that this bug will potentially manifest in formatting errors for other regions (AR and AU for example). The XPATH query I used to find those is //territory[availableFormats/numberFormat[count(leadingDigits)>1]]
...wrapping that in a count() gives 22 matching territories back.

If I understand correctly, GlobalPhone is only using the leadingDigits information to do formatting. That is, based on a global search for "leadingDigits" in the solution, it seems that the information is only being used in the Format class. This means that parsing should not be affected (good news).

At the moment, the leadingDigits value in the format object is being set in the CompileFormat method of the DatabaseGenerator class. It gets the value from the Pattern method , which in turn calls TextOrNull, which is where the buggy "concatenation" of text from multiple leadingDigits nodes seems to be happening.

I see a two possible ways to fix this...

1. Appreciate that there may be more than one leadingDigits node, which will probably require a change to the *.json database structure (i.e. leadingDigits would no longer just be a string value, but rather an array of strings or similar). Depending on how this is implemented this may be a breaking change (i.e. consider the case where existing users update GlobalPhone but keep an old *.json database with the old structure).
2. Keep the leadingDigits property as a string, but do some regex tweaking to represent all the possible formatting alternatives in the one regex. That is, a regex something like this...
   (?:leadingDigitsRegex1)|(?:leadingDigitsRegex2)
   ...where leadingDigitsRegex1 is the regex coming from the first leadingDigits node, leadingDigitsRegex2 is the regex coming from the second leadingDigits node etc.
   Seeing as the leadingDigits are only being used for matching purposes in the Format class, this seems like it would work (note that I'm no regex expert...there might be a better way to do it than the way I've described above).

Although approach #2 is perhaps simpler to implement, #1 is perhaps the less-hacky solution. I'll leave it to you guys to decide the appropriate direction. Maybe you also have other approaches that are more appropriate.

from globalphone.

I would prefer that we change the database structure. #1 is definitely the less hackish solution. If we do a major version change then an update to the format is more than acceptable.

from globalphone.

Fixing the format does not fix the bug.

from globalphone.

The format is fixed.

from globalphone.