Add method to capitalize person names

danielstjules / Stringy

A PHP string manipulation library with multibyte support

MIT License

2.46k stars 216 forks source link

Add method to capitalize person names #141

Closed yhoiseth closed 8 years ago

yhoiseth commented 8 years ago

Hi @danielstjules,

Thanks for Stringy.

I added a method for capitalizing person names. Imagine that a user enters his name in lowercase, e.g. 'jaap de hoop scheffer', and that you'd like to capitalize it. Now, 'jaap', 'hoop' and 'scheffer' should be capitalized, but not 'de'. A naïve approach where every word is capitalized would result in wrong capitalization ('de' would be capitalized). Stringy->capitalizePersonName() takes care of that by checking the string to be capitalized against a list of special cases such as 'de'.

Can you please review this pull request? I'll be happy to fix any issues.

Some things to keep in mind for the review:

I'm not very familiar with Stringy, so I may have made some less-than-ideal architectural choices, duplicated some code, etc.
I'm inexperienced with character encoding.

Overall, I'm quite inexperienced as a developer, and I made this pull request partly as a learning experience. Therefore, I'd deeply appreciate feedback on my contribution.

yhoiseth commented 8 years ago

It seems like there's an issue with the encoding 'ISO-8859-1'.

The tests pass when I simply run phpunit in the project root.

I've tried adding the encoding to the tests, so that StringyTest:2441 looks like this: array('Marcus Aurelius', 'marcus aurelius', 'ISO-8859-1'),, but the tests still pass.

Any suggestions as to how I can reproduce the failing tests?

danielstjules commented 8 years ago

I appreciate the PR! I don't think Stringy's core is the place to hold this kind of logic though. It's a bit language specific, and something that would be better suited in an inflector library like https://github.com/doctrine/inflector Have you thought of extending a different library, or perhaps releasing your own extension to Stringy? :)

yhoiseth commented 8 years ago

Thanks for the feedback and suggestions.

I considered Doctrine Inflector, but it doesn't seem that active, and I couldn't find any documentation. Therefore, I decided to make a Stringy extension.

Would you mind taking a look and letting me know if there's anything you'd change?

danielstjules commented 8 years ago

Might be more appropriately named StringyInflector, that way it could support additional functionality if interested :)

This does make me realize that it would be nice if Stringy had an API for extending its functionality rather than resorting to inheritance.

Aside from that, no recommendations of the code itself!

yhoiseth commented 8 years ago

Might be more appropriately named StringyInflector, that way it could support additional functionality if interested :)

Good idea, thanks. I went ahead and did just that. Can you please add StringyInflector to the list of Stringy extensions?

This does make me realize that it would be nice if Stringy had an API for extending its functionality rather than resorting to inheritance.

Interesting. I can imagine that inheritance could be a problem if we were to install multiple extensions and they required different versions of Stringy, right.

How would an API for extending Stringy's functionality work? Perhaps you know of an example I can have a look at?