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

Pluralization is broken #4510

Open
ShahriarKh opened this issue Apr 12, 2024 · 7 comments
Open

Pluralization is broken #4510

ShahriarKh opened this issue Apr 12, 2024 · 7 comments
Assignees
Labels
Priority: Low Something worth considering, but not a top priority for the team Type: Enhancement Improving an existing feature

Comments

@ShahriarKh
Copy link
Contributor

ShahriarKh commented Apr 12, 2024

Package version

6.2.2

Describe the bug

When I want to create new models or controllers (and probably other things), the name formatter behaves wrongly in some cases.
For example, try:

node ace make:model cookies

This creates cooky.ts!

This happens for many other words, including made-up words.

I investigated a bit, and seems like the problem comes from pluralize library which is used under the hood by poppinss/utils. Pluralize was last updated in 2019, and has many open pull requests.

So, 3 possible solutions come to my mind:

  1. replace pluralize or fork it to fix this bugs and avoid conversion of made-up words
  2. add a flag for all make:* commands to prevent this auto fix.
  3. instead of changing the word automatically, add an extra step in cli to ask users if they want their new controller/model/... name to be changed or not... something like:
    Do you want to change "CookiesCOntroLLer" to "cooky"? (Y/n)

Reproduction repo

No response

@kyr0
Copy link

kyr0 commented Apr 12, 2024

Don't try to switch to the fancy compromise library; which is an amazing lib.. but it gets this wrong too. I was about to post that this would be a great alternative, but it's not. Demo: https://codesandbox.io/p/sandbox/compromise-forked-h2vg55

@thetutlage
Copy link
Member

Not sure what really the solution is, if most of the popular libs get it wrong.

But I agree, bypassing the transformation with a flag seems like a nice middle ground. Not in favor of a prompt.

@thetutlage thetutlage self-assigned this Apr 22, 2024
@thetutlage thetutlage added Type: Enhancement Improving an existing feature Priority: Low Something worth considering, but not a top priority for the team labels Apr 22, 2024
@ShahriarKh
Copy link
Contributor Author

Or maybe some config in package.json or elsewhere:

"autoFixNames": {
  "controllers": "always",
  "models": "never",
  ...
}

This also introduces the possibility of letting devs choose their naming conventions. For example:
"models":"PascalCase"

I know it makes it more complex to implement, but it has its use cases.

@RomainLanz
Copy link
Member

Or maybe some config in package.json or elsewhere:

"autoFixNames": {
  "controllers": "always",
  "models": "never",
  ...
}

This also introduces the possibility of letting devs choose their naming conventions. For example: "models":"PascalCase"

I know it makes it more complex to implement, but it has its use cases.

People can already change all scaffolding option https://docs.adonisjs.com/guides/scaffolding

@ShahriarKh
Copy link
Contributor Author

Oh, so I think the flag is the way to go. The CLI should either use defined (or default) naming conventions and do the autofix, or skip them and use the provided name if a flag is present.

@kyr0
Copy link

kyr0 commented Apr 23, 2024

The best option would probably be, to impement and release a decent pluralization lib based on wordnet dataset, or else. Idk if I'll find the time to do that, but that's basically the only accurate solution. Grammar rules won't ever catch all the fish there is. It could be fun to optimize this by computing a xor checksum on the diff of the dataset that works well by grammar rule computation (optimization on-disk index size), and have a lookup table for all else words that fail. Re-building the lib automatically when a new mistake is reported, for maintainers to quickly include fixes from upstream. Would be fun.. and a nice example for my brotli compress lib xD

@rnwonder
Copy link

Seems it was fix on pluralize github version not on the npm version according to this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low Something worth considering, but not a top priority for the team Type: Enhancement Improving an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants