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

[9.x] Removes redundant uncountable words and change language for pluralizer ruleset #41891

Closed
wants to merge 2 commits into from
Closed

Conversation

cesarep
Copy link
Contributor

@cesarep cesarep commented Apr 8, 2022

Hi!

As seen in https://github.com/doctrine/inflector/blob/2.0.x/lib/Doctrine/Inflector/RulesetInflector.php#L37
when inflecting a word, either pluralizing or singularizing, Inflector calls $ruleset->getUninflected()->matches($word)
to check if the inflected word is in https://github.com/doctrine/inflector/blob/2.0.x/lib/Doctrine/Inflector/Rules/English/Uninflected.php
or the corresponding language Uninflected file, if it is, it returns the very same word, unchanged.

There are only 7 words not directly listed there, those beeing: 'cattle', 'firmware', 'hardware', 'kin', 'recommended', 'related', 'software', from those, yield new Pattern('\w+ware$') on line 42 matches firm... hard... and soft...ware.

I already created a pull request to add those there doctrine/inflector#194

There's really no reason to keep them. New words should be requested on that Uninflected file, for all the available languages.

Now, as for the language ruleset, I'm not really sure if it's allowed to call Container::getInstance() inside Illuminate/Support, the only other file that does this is Traits/Localizable.php but it works fine locally on my machine.

With that, we can add a 'pluralizer_ruleset' config on app.php to choose between 'english', 'french', 'norwegian-bokmal', 'portuguese', 'spanish', 'turkish'

I believe this change is backward compatible with older versions, i tested on my 8.x version

@driesvints driesvints changed the title [8.x & 9.x] Removes redundant uncuntable words and change language for pluralizer ruleset [9.x] Removes redundant uncuntable words and change language for pluralizer ruleset Apr 8, 2022
@willrowe
Copy link
Contributor

willrowe commented Apr 8, 2022

May want to spellcheck the PR name...

@driesvints driesvints changed the title [9.x] Removes redundant uncuntable words and change language for pluralizer ruleset [9.x] Removes redundant uncountable words and change language for pluralizer ruleset Apr 8, 2022
@cesarep
Copy link
Contributor Author

cesarep commented Apr 8, 2022

Oh my... Well the checks sure don't liked it

@driesvints
Copy link
Member

Given the failing tests, I don't think we should go ahead with this.

@cesarep cesarep marked this pull request as draft April 8, 2022 13:27
use Doctrine\Inflector\InflectorFactory;

class Pluralizer
{
/**
* Uncountable word forms.
*
* @var string[]
* New uncuntable words should be added to: https://github.com/doctrine/inflector/blob/2.0.x/lib/Doctrine/Inflector/Rules/English/Uninflected.php
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* New uncuntable words should be added to: https://github.com/doctrine/inflector/blob/2.0.x/lib/Doctrine/Inflector/Rules/English/Uninflected.php
* New uncountable words should be added to: https://github.com/doctrine/inflector/blob/2.0.x/lib/Doctrine/Inflector/Rules/English/Uninflected.php

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, as said doctrine/inflector#194 (comment), "related" and "recommended" are not nouns, perhaps the uncountable array and function should remain, but just for these edge cases, not nouns, nor words that already exist on the Uninflected file, with the recommendation to add new nouns to the inflector repository

@driesvints
Copy link
Member

I'm going to close this one for now. You're free to attempt again with passing tests but honestly I think we should not go ahead with this. Thanks

@driesvints driesvints closed this Apr 8, 2022
@cesarep
Copy link
Contributor Author

cesarep commented Apr 8, 2022

Oh ok, I'm trying to run these tests locally but i keep getting database errors, how should i setup my environment to do so?
I couldn't find anything in the Contribution Guide regarding that

@driesvints
Copy link
Member

Not sure why it doesn't work for you sorry

@cesarep
Copy link
Contributor Author

cesarep commented Apr 10, 2022

Hi!
So, i managed to get the tests working! I changed SupportPluralizerTest.php itself adding the getEnvironmentSetUp($app) function, that was present on all the other tests on Classes that depend on configs, setting the required config there. But ultimately (and it feels like cheating), I checked if the Container instance had the config class with $app->has('config'), setting the language to 'english' if it didn't, effectively bypassing the Target class [config] does not exist. error. Is it allowed tho?

@cesarep
Copy link
Contributor Author

cesarep commented Apr 11, 2022

Hey @driesvints!

Should a try a new PR now with the tests working? cesarep@74639eb
Or this is not how nor where $app->has() should be used? Because i haven't seen it like that anywhere else, but the tests didn't complaint

@driesvints
Copy link
Member

@cesarep I still feel that we should not go ahead with this PR but you're always free to try to see if you can change Taylor's mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants