Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First version of Dutch Lang #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

micschk
Copy link

@micschk micschk commented Jun 18, 2018

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.

@wyrfel wyrfel self-assigned this Jun 18, 2018
Copy link
Contributor

@wyrfel wyrfel left a comment

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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){
Copy link
Contributor

Choose a reason for hiding this comment

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

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){
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you decide these aren't needed, please drop these two lines.

// $key = str_replace('’', '', $key);
$prefixMap[$key] = $prefix;

// add version with ' instead of ’
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@rvanlaak
Copy link

rvanlaak commented Nov 5, 2019

@micschk are you ok if we finish your PR in order to get it merged?

@micschk
Copy link
Author

micschk commented Nov 5, 2019

@rvanlaak That'd be great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants