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

Singularize regattas to regatta instead of regattum #243

Merged
merged 2 commits into from
Feb 18, 2024

Conversation

discern
Copy link
Contributor

@discern discern commented Feb 13, 2024

While using Laravel Shift's Blueprint, I encountered an issue with the word "regatta" not being inflected properly (the singularizer was changing the plural from "regattas" to "regattum"). The updated pattern should solve it in this case. I added a few other words to the test, as well.

@discern discern changed the title Singularize regattas to regatta. Test a few similar words. Singularize regattas to regatta instead of regattum Feb 13, 2024
@@ -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!

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 :)

@malarzm malarzm added this to the 2.0.10 milestone Feb 13, 2024
@malarzm malarzm added the Bug label Feb 13, 2024
@malarzm malarzm merged commit 5817d06 into doctrine:2.0.x Feb 18, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants