ordinal.js icon indicating copy to clipboard operation
ordinal.js copied to clipboard

Instead of using `indicator`, pass a `boolean` as the second param.

Open joshuapinter opened this issue 7 years ago • 6 comments

Maybe something like this:

// Method definition: ordinal( number, includeNumber = true ).

ordinal(2);        // "2nd"
ordinal(2, false); // "nd"
ordinal(2, true);  // "2nd"

Defaults to include the number but you can pass false to the second param and it will not include the number.

Thoughts?

joshuapinter avatar Jun 18 '17 14:06 joshuapinter

@joshuapinter I suppose, what is the usecase/advantage though?

dcousens avatar Jun 18 '17 14:06 dcousens

Just more intuitive, instead of having to import something else called "indicator" which is not very self-explanatory.

In my case, which probably isn't too uncommon, sometimes you need to include the number and sometimes you don't.

Happy to do a PR if you are open to the idea.

joshuapinter avatar Jun 18 '17 14:06 joshuapinter

indicator is truly a terrible name, it is more a "suffix-only" option - alas this API was mimicking the previous versions of this module (which I inherited from someone else).

I'm not against the idea that the existing is bad, but, I don't think the proposed syntax above is anymore intuitive.

Defaulted positional arguments can be messy too if users aren't careful, think parseInt and Array.prototype.map.

dcousens avatar Jun 18 '17 14:06 dcousens

Ha. We agree on that.

I'm not a fan of unnamed params passed in either, actually.

What about an options Object as the second params with suffixOnly, like this:

ordinal( 2 );                      // "2nd"
ordinal( 2, {suffixOnly: true} );  // "nd"
ordinal( 2, {suffixOnly: false} ); // "2nd"

joshuapinter avatar Jun 18 '17 15:06 joshuapinter

@joshuapinter I like it. It also easily allows for language extensions if anyone ever needs it.

PR accepted, as 2.0.0

dcousens avatar Jun 18 '17 15:06 dcousens

👍 I'll take a look when I get time.

joshuapinter avatar Jun 18 '17 15:06 joshuapinter