Conversation
wyrfel
left a comment
wyrfel
left a comment
There was a problem hiding this comment.
Wow, that is some comprehensive work. Thank you. Can you please add a unit test for this class and look at the inline comments i made? Thank you.
|
|
||
| public function getSuffixes(): array | ||
| { | ||
| return self::SUFFIXES; |
There was a problem hiding this comment.
Since you are not doing any transformations in here, the mappers would try to match the suffixes by the keys provided in the array above (see https://github.com/theiconic/name-parser/blob/master/src/Mapper/SuffixMapper.php#L42). This means the input would have to contain e.g. bacc.alaureus for the suffix to be recognised and it would then be normalised to 'bacc.'. I'm pretty sure that's not what you'd want and a solution like the array_combine below, with provision of properly normalised array values could be a solution.
| public function getLastnamePrefixes(): array | ||
| { | ||
| $prefixMap = []; | ||
| foreach(self::LASTNAME_PREFIXES as $prefix){ |
There was a problem hiding this comment.
Please add spaces before and after the parenthesis to keep a consistent code style.
| $prefixMap[$key] = $prefix; | ||
|
|
||
| // add version with ' instead of ’ | ||
| if(strpos($prefix, '’') !== false){ |
There was a problem hiding this comment.
Please add a space after if to keep consistent code style.
| foreach(self::LASTNAME_PREFIXES as $prefix){ | ||
| $key = strtolower($prefix); | ||
| // $key = str_replace(' ', '-', $key); | ||
| // $key = str_replace('’', '', $key); |
There was a problem hiding this comment.
If you decide these aren't needed, please drop these two lines.
| // $key = str_replace('’', '', $key); | ||
| $prefixMap[$key] = $prefix; | ||
|
|
||
| // add version with ' instead of ’ |
There was a problem hiding this comment.
We prefer to let the code speak for itself unless a comment is required to explain the background/reasoning of the implementation. I would suggest to drop this comment.
|
@micschk are you ok if we finish your PR in order to get it merged? |
|
@rvanlaak That'd be great, thanks! |
3 participants
I've created a Dutch version, it's just not totally clear to me what's the purpose & specific requirements to the keys of the language arrays. I figured they're just references but they may have something to do with 'normalizing'(?) Not sure, hope they're OK like this.