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

Plural should not have indefinite article #23

Closed
gajus opened this issue Mar 22, 2020 · 5 comments
Closed

Plural should not have indefinite article #23

gajus opened this issue Mar 22, 2020 · 5 comments

Comments

@gajus
Copy link

gajus commented Mar 22, 2020

// current
indefinite('French Fries') => 'a French Fries'

// expected
indefinite('French Fries') => 'French Fries'
@gajus
Copy link
Author

gajus commented Mar 22, 2020

Quick workaround:

// @flow

import indefinite from 'indefinite';
import pluralize from 'pluralize';

export default (subject: string): string => {
  if (pluralize(subject) === subject) {
    return subject;
  }

  return indefinite(subject);
};

@tandrewnichols
Copy link
Owner

Currently indefinite doesn't attempt to know whether something should have an indefinite article or not, only which one to use (a vs an). To this point, I've relied on consumers to know whether it should or shouldn't have one (and call indefinite accordingly). For that reason, I'm curious to hear more about your use case. Is there a reason you can't just not call it? I could probably support it using your workaround, but I don't know if I should . . . I don't know if it's within the scope of the project because

  1. I have to pull in pluralize, which doesn't matter for the server-side, but adds a lot of weight to the client-side that everyone has to download but most won't use.
  2. I'm then relying on pluralize not to get it wrong. Like, can I know with 100% certainty that pluralize(subject) === subject will always work? What about for irregulars?
  3. Maybe for separation of concerns it's better to let consumers handle it (as you've done).

To that last point, it's obviously sort of related, so I can see a case for including it, but the other two points seem prohibitive at the moment. If I added this, I think I'd maybe not want to use pluralize, but then I'm not sure I want to try to maintain whatever I wrote in its place.

@gajus
Copy link
Author

gajus commented Mar 23, 2020

Just document the workaround in the docs. No need to depend on pluralize for the reasons you've suggested.

@tandrewnichols
Copy link
Owner

Sounds good. Thanks.

@tandrewnichols
Copy link
Owner

Updated the readme.

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

No branches or pull requests

2 participants