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.

Sign up for GitHub

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

Jump to bottom

Singularize regattas to regatta instead of regattum #243

Merged
merged 2 commits into from
Feb 18, 2024
Merged

Singularize regattas to regatta instead of regattum #243

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ad66b2
Singularize regattas to regatta. Test a few similar words.
Feb 13, 2024
226fc22
Add one more regatta assertion
discern Feb 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/Doctrine/Inflector/Rules/English/Inflectible.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public static function getSingular(): iterable
yield new Transformation(new Pattern('(analy|diagno|^ba|(p)arenthe|(p)rogno|(s)ynop|(t)he)ses$'), '\1\2sis');
yield new Transformation(new Pattern('(tax)a$'), '\1on');
yield new Transformation(new Pattern('(c)riteria$'), '\1riterion');
yield new Transformation(new Pattern('([ti])a$'), '\1um');
yield new Transformation(new Pattern('([ti])a(?<!regatta)$'), '\1um');
Copy link
Member

Choose a reason for hiding this comment

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

Given you're fixing one word only I think it'd be better to put this into irregular list here. New tests will be great addition though!

inflector/lib/Doctrine/Inflector/Rules/English/Inflectible.php

Line 93 in 2930cd5

public static function getIrregular(): iterable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the first thing I tried, but it only worked in one direction. With yield new Substitution(new Word('regatta'), new Word('regattas')); in the irregular list, Str::singular('regatta') produces "regattum", and with yield new Substitution(new Word('regattas'), new Word('regatta'));, Str::singular('regatta') produces "regattas." For that reason I used a negative look-behind in the getSingular() regex. Open to other suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

Str::singular('regatta') produces "regattum"

That's not a blocker, we do not support singularizing words which are already singular :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think perhaps the inflector is assuming that "regatta" is plural, due to the -ta ending. Besides, shouldn't it just return the original word if it wasn't inflectible in the requested direction?

I added these two assertion arguments to dataSingularsUninflectedWhenSingularized().

['regatta', 'regattum'],
['regattas', 'regattum'],

Without changing the rules, and with the added assertions:

'regatta' should not be singularized to 'regattum'

By adding this to getIrregular():

getIrregular()
    yield new Substitution(new Word('regatta'), new Word('regattas'));
...
'regatta' should not be singularized to 'regattum'

(Oddly, by reversing these arguments, the tests pass. But this makes no sense, and Blueprint fails anyway.)

By changing the regex, the tests pass and Blueprint builds the table and class names correctly:

getSingular()
    yield new Transformation(new Pattern('([ti])a(?<!regatta)$'), '\1um');

But then we'd be changing a regex for a rarely used word, which has been deemed suboptimal.

Again, if you have a better idea, I'm all ears. Otherwise, I guess I'll use my fork.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for all the tinkering and explanations! I wanted to make sure we're not introducing regexes without a good reason. So without further ado :)

yield new Transformation(new Pattern('(p)eople$'), '\1\2erson');
yield new Transformation(new Pattern('(m)en$'), '\1an');
yield new Transformation(new Pattern('(c)hildren$'), '\1\2hild');
Expand Down
8 changes: 8 additions & 0 deletions tests/Doctrine/Tests/Inflector/Rules/English/EnglishFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,9 +233,11 @@ public function dataSampleWords(): array
['old_news', 'old_news'],
['olive', 'olives'],
['ox', 'oxen'],
['pactum', 'pacta'],
['pants', 'pants'],
['pass', 'passes'],
['passerby', 'passersby'],
['pasta', 'pastas'],
['patience', 'patience'],
['pekingese', 'pekingese'],
['person', 'people'],
Expand Down Expand Up @@ -264,6 +266,7 @@ public function dataSampleWords(): array
['radius', 'radii'],
['rain', 'rain'],
['reflex', 'reflexes'],
['regatta', 'regattas'],
['research', 'research'],
['rhinoceros', 'rhinoceros'],
['rice', 'rice'],
Expand Down Expand Up @@ -314,6 +317,7 @@ public function dataSampleWords(): array
['stimulus', 'stimuli'],
['stitch', 'stitches'],
['story', 'stories'],
['stratum', 'strata'],
['sugar', 'sugar'],
['swine', 'swine'],
['switch', 'switches'],
Expand Down Expand Up @@ -387,8 +391,12 @@ public function dataSingularsUninflectedWhenSingularized(): array
['utopia', 'utopium'],
['sepia', 'sepium'],
['mafia', 'mafium'],
['regatta', 'regattum'],
['pactum', 'pactums'],
['fascia', 'fascium'],
['status', 'statu'],
['stratum', 'strati'],
['stratum', 'stratums'],
['campus', 'campu'],
['axis', 'axes'],
];
Expand Down
Loading