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

Enhancement for company names, titles and lastname extensions #40

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

blacksenator
Copy link

Fix #38

Differentiation between companies and personal names, more detailed mappings for prefixes (multiple words/components), titles (mainly academic with multiple words/components) and lastname extensions (nobilit predicates)

@coveralls
Copy link

coveralls commented Dec 15, 2019

Coverage Status

Coverage decreased (-5.0%) to 95.023% when pulling 92bdf0c on blacksenator:enhancement_branch into 9a54a71 on theiconic:master.

…ed names and their separators, where the keys are representing vCard properties according to RFC6350
@blacksenator
Copy link
Author

Okay, I used three commits to achieve the desired goal. But now I'm happy with the result. It's too bad that the automatic test routines have such narrow limits.
To be honest, I can't see where I can make the code even leaner so as not to exceed the limits (but I´m not a pro :/ ).
I would appreciate a thorough manual review and feedback. I would be even more pleased if my additions become part of the solution.

@rvanlaak
Copy link

rvanlaak commented Dec 16, 2019

Nice! Will work on Dutch.php asap to verify your implementation

I see you're also adding a PHPStan config in this PR. It would be good to add that in a separate PR, and run it via Github Actions.

You can check php-translation/symfony-storage#38 as example 👍

@blacksenator
Copy link
Author

blacksenator commented Dec 16, 2019

@rvanlaak Thank you for your fast reaction.
I'm afraid I'm not familiar with GitHub Actions. I see the advantages of all automation, but it is difficult for me to find my way around in the tools - I am probably too old for this stuff...
I added the line because I received this as a false report for my German.php.
As long as the PR is not merged, the line does not need to be added - right?

@rvanlaak
Copy link

@blacksenator I will add PHPStan in a separate PR 👍

Would be best to remove as many unrelated files from this PR, so more tests pass.

I'm also affraid that it would be best to split this PR in two. One for the titles / lastname extensions. And one for the company names.

And, to get code coverage it probably is enough to add a test case in NameTest, ParserTest and GermanParserTest.

@rvanlaak
Copy link

see #41

@blacksenator
Copy link
Author

@wyrfel Do you agree with rvanlaak to split up this PR into two or maybe three PR´s? It is of course possible to split up the changes, but I would like to avoid the effort if it is not necessary.

@wyrfel
Copy link
Contributor

wyrfel commented Dec 18, 2019

Hey @blacksenator . Like i said on the issue, i think some of this functionality is out of scope. I also feel this PR is implementing some things that are already there. I'll review and get back to you, though. In the meantime, yes, i agree that splitting into separate PRs for the different aspects would be good, e.g. one for PHPStan, one for extending the german language file etc. . That way we can more easily discuss the different aspects.

@wyrfel
Copy link
Contributor

wyrfel commented May 23, 2020

@blacksenator I would like to integrate a lot of these changes into a next major release. For that to happen we will need you to remove the author, copyright and licence annotations in the code, please. We will credit you in the release notes and github provides transparency over the authorship, as well.

@blacksenator
Copy link
Author

done

@rvanlaak
Copy link

The additional methods on LanguageInterface will be a BC break for all current implementations. Can we find a way to make this forward compatible, for example by introducing a new interface and adding deprecating notices on the old one?

@blacksenator
Copy link
Author

Is that a question for me? If so, then I have to admit that I did not understand what your idea for a workaround should look like

@wyrfel
Copy link
Contributor

wyrfel commented May 26, 2020

@rvanlaak I'm planning to integrate this into a new major release that will have the LanguageInterface renamed to DefinitionInterface, anyway (see #46). It will also have multibyte-safe string operations. I'm afraid you will have to make the necessary adjustments when upgrading to that newer version, once ready. Do you anticipate major problems from this?

@rvanlaak
Copy link

What about releasing a minor version that adds DefinitionInterface already?

If you would keep both LanguageInterface and DefinitionInterface in place and make all related functionality BC, then Parser can check which one is used to trigger the deprecation. The LanguageInterface then is mentioned to be removed in the major version.

This will give you time to release DefinitionInterface specific functionality already and decouple that from the BC break.

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.

Output in vCard (RFC6350) matching form
4 participants