Add locale option
#75
Conversation
…ted methods. - Added tests. - Updated type file and documentation.
|
Very much 👍. This is the proper solution here. @sindresorhus I know your stance on adding options, but I think this is a valid exception to that rule. @rasgele Could you potentially add tests for cases where the output differs between locales? |
|
@Qix- there are such tests in |
readme.md
Outdated
| Type: `string | string[]`\ | ||
| Default: `undefined` | ||
|
|
||
| Locale to be used with lowercase or uppercase conversions. |
There was a problem hiding this comment.
This needs to explicitly say what the expected string locale format it. (for example, what en-US refers to).
There was a problem hiding this comment.
It's also unclear why one would want to specify multiple locales and how that works.
There was a problem hiding this comment.
I updated with an explicit reference to String.prototype.toLocaleUpperCase() and excerpt from that page. It explains format and usage with multiple locale.
Details (like expected format being BCP 47 etc.) seemed too much in this context, but it is up to you.
There was a problem hiding this comment.
One example in Usage and all in the API docs for locale.
index.d.ts
Outdated
| //=> 'loremİpsum' | ||
|
|
||
| camelCase('lorem-ipsum', {locale: ['en-US', 'en-GB]}); | ||
| //=> 'loremIpsum' |
There was a problem hiding this comment.
The readme and index.d.ts should be fully in sync.
There was a problem hiding this comment.
I am a little confused about this. I interpreted your next comment as "Keep a single example in index.d.ts and move others to readme.md file", which breaks sync.
The |
Fixed. |
|
|
||
| camelCase('lorem-ipsum', {locale: ['tr', 'TR', 'tr-TR']}); | ||
| //=> 'loremİpsum' | ||
| ``` |
There was a problem hiding this comment.
The readme and index.d.ts should be fully in sync.
There was a problem hiding this comment.
readme contains 4 examples and index.d.ts contains one example due to your previous comment: #75 (comment)
Do you means smth else?
There was a problem hiding this comment.
These 4 examples should be here: https://github.com/sindresorhus/camelcase/pull/75/files#diff-b52768974e6bc0faccb7d4b75b162c99R14 (Just like the readme).
There was a problem hiding this comment.
Oh, ok. thanks. Done.
index.d.ts
Outdated
| /** | ||
| From `String.prototype.toLocaleUpperCase()`: The locale parameter indicates the locale to be used to convert to upper/lower case according to any locale-specific case mappings. If multiple locales are given in an Array, the best available locale is used. The default locale is the host environment’s current locale. | ||
|
|
||
| @default The host environment’s current locale |
There was a problem hiding this comment.
This is incorrect syntax. @default only works with a JS value.
There was a problem hiding this comment.
Then I'll remove @default and append following to the description of locale parameter:
"If not provided, host environment's current locale is used".
Do you have other suggestion for such a case?
There was a problem hiding this comment.
You can just write Default: The host environment’s current locale.
- Removed duplicate specification for default.
|
Hi @sindresorhus, @rasgele any progress on this PR? I'm also having problems with Jest. |
|
There's still feedback that needs to be addressed. |
|
Sorry, I missed latest comments. |
- Fixed incorrect @default usage.
|
@sindresorhus Employed 'request a review' as a 'ping'... sounded appropriate :) |
Fixes #72
Added locale property to option parameter along with tests and doc.