plural to singular conversion of "add" and "remove" methods #627
Conversation
…eters names As said in the TODO at line 707 "This needs actual plural -> singular conversion". So this patch uses Symfony\Component\PropertyAccess\StringUtil::singularify() to make a singular from the property name. Note that singularify() may return an array when multiple singulars are possible. But as the plural map in singularify() is ordered by plural weight, most of the time the first singular in the array is ok. At least it covers most cases and uses standard Symfony tool.
|
This is also missing the addition of @doctrine/team-doctrine2: This seems applicable to the ORM as well. Perhaps this functionality belongs in common? |
|
no problem, i'll make these changes asap. thanks for your feedback |
|
@jmikola we still lack a generator package for this sort of stuff. I think that for such a small and edge-case feature, introducing an additional dependency is a no-go. @NicolasMassart besides CS, this PR is missing tests |
|
@Ocramius: Afterwards, I noticed that common had an Inflector class for stuff like this. Is this our alternative? |
|
@jmikola yes, |
|
Ok, let me some time, i'll look how I can make this less demendant and clean (tests and standards). |
|
Hi, I'm ok to use the Inflector class from Doctrine, but the one in the already dependent doctrine/common project don't do plural to singular conversion but only classes name formating. To have plural to singular conversion, it would be necessary to use the class from the doctrine/inflector project ( https://packagist.org/packages/doctrine/inflector ) and thus add a dependency on a dev version. Does any of you have an advice about how to deal with that ? When this problem will be fixed, I'll then make it CS compliant and full of tests, I promise :) Thanks, (Sorry for this question, but I'm pretty new to Symfony, doctrine and co... and also making PR on github... and I admit that it's my first try to be helpful on a public project infact... but please, I don't say that for making you forgiving, just to explain the reason why I'm trying to find the right way to help with this contribution). |
|
@NicolasMassart it's already awesome to see some will to contribute! Just don't be too frustrated at the beginning: it takes some time to start getting own suggestions integrated into other people's projects (we're picky!) I think the dev requirement is ok assuming that we'll get a tag on the inflector as soon as we need it. |
|
@Ocramius: Is Inflector one of the libraries split off from common in 2.4? |
|
@jmikola yepp |
|
So if adding a dependency to doctrine/inflector is ok, I'll change my code to use it and propose my patch. |
|
@NicolasMassart: go ahead with that. For now, you can use Doctrine/Common/Util/Inflector, which should already be utilized elsewhere in ODM. In Common 2.4, that class is a stub that extends the base class in doctrine/inflector, since the library was moved off to its own repository. |
Unit tests works. Remaining code style cleanup to do when the dependecy problem will be solved see doctrine#627
|
Hi, back from holidays I'm getting back on this patch. However, I made my modification using directly "doctrine/inflector": "1.0.*@dev" in the composer.json to test. My code seems ok (except CS conformance for now, I'll fix it when it will be technically ok) and new assertions to test plural form in unit tests works too. You can check it on my last commit on my fork NicolasMassart@e82d08d So there's just this dependency problem remaining. |
|
@NicolasMassart: https://github.com/doctrine/common/blob/master/lib/Doctrine/Common/Util/Inflector.php is being kept around in doctrine/common 2.4 for BC reasons, so you should be able to continue using that and have this PR work with older and newer versions of doctrine/common. Am I mistaken? |
|
Hi, it works, but only if I add the "minimum-stability": "dev" in the doctrine/mongodb-odm composer.json |
|
@NicolasMassart: My mistake! I didn't realize 2.3's Inflector was missing all of the pluralization support in doctrine/inflector (and 2.4). I see that doctrine/inflector has a 1.0 stable release, so perhaps you can depend directly on that and use its new class name (which doesn't conflict with doctrine/common 2.3 and the BC class in 2.4). Does that work? |
|
Yes, it's what I did in NicolasMassart@e82d08d |
| @@ -15,7 +15,8 @@ | |||
| "php": ">=5.3.2", | |||
| "symfony/console": "~2.0", | |||
| "doctrine/common": ">=2.2.0,<2.5-dev", | |||
| "doctrine/mongodb": "1.0.*" | |||
| "doctrine/mongodb": "1.0.*", | |||
| "doctrine/inflector": "1.0.*@dev" | |||
There was a problem hiding this comment.
There is a v1.0 tag for doctrine/inflector, so I think we can just depend on ~1.0 instead of a dev version.
|
@NicolasMassart: I left two more comments above, but I think we're good. I'll merge after the aforementioned changes are implemented. |
Coding standards applied to spaces around conditions and newline at end of test file Modified assertion from EntityGeneratorBook class to DocumentGeneratorBook class (on my addition and also on an already present assertion.)
|
I made the dependency and code standard modifications. I also detected a problem on the test file and modified assertion from "EntityGeneratorBook" class to Nicolas. |
| $methodName = substr($methodName, 0, -1); | ||
| if ( in_array($type, array('add', 'remove')) ) { | ||
| $methodName = Inflector::singularize($methodName); | ||
| $variablename = Inflector::singularize($fieldName); |
There was a problem hiding this comment.
Previously, $variableName was always camelized, which means that its underscores were always replaced. Now, it will only be given a singular form for add*() and remove*() methods, which seems inconsistent. I think it should always be run through Inflector::camelize() (below the classify() call for $methodName) and then conditionally singularized here.
|
@NicolasMassart: Dependency changes look good, but I took another read through the code and posted some comments. |
|
Yep, I saw it. |
Also fixed a variable name case
|
Hi, I rewrote it in a more logical way, I hope. I used a ternary and a temporary variable so that all the code after it is common. I hope this is the right one ;) |
|
Thanks for the work on this. I added a commit on top with some refactoring further down in the method and rebased this PR into a single commit (merged in 2c32111). |
plural to singular conversion of "add" and "remove" methods and parameters names
As said in the TODO at line 707 "This needs actual plural -> singular conversion".
So this patch uses Symfony\Component\PropertyAccess\StringUtil::singularify() to make a singular from the property name.
Note that singularify() may return an array when multiple singulars are possible. But as the plural map in singularify() is ordered by plural weight, most of the time the first singular in the array is ok. At least it covers most cases and uses standard Symfony tool.