-
Notifications
You must be signed in to change notification settings - Fork 38
feat: add plural support #284
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
base: master
Are you sure you want to change the base?
Conversation
PORTAL-6292 | @jkalberer | Add plural support to babel i18n extractor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this work. The PR is not too large so that's fine, but I'd appreciate more atomic PRs in the future, because my brain is no longer "hot" on that project. Anyways.
Remove the opinionated logic around ***_one derived keys. These were being set as "not derived" and given specific logic to populate them with default values. This doesn't make a lot of sense since we generally will want to add values to all derived keys or none of them.
I don't want to bikeshed this and I don't have strong feeling against it, but as it is sort of a breaking change (arguably one I'd be fairly confident making without a major version bump), I'd prefer stronger incentives. I can't say I'm very convinced by the "it clearly doesn't make sense because generally <some projection on what people are doing>" take…
Why did you implement it? Did it make code easier to write? Do you consider the old behavior a bug? I don't really see what value this change brings.
@@ -156,8 +158,9 @@ export function computeDerivedKeys( | |||
locale, | |||
// TODO: a comment hint should allow to override cardinality/ordinality. | |||
// It defaults to cardinal, but this is not correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be amended accordingly.
@@ -1,5 +1,6 @@ | |||
/.yarn/ | |||
/.pnp* | |||
/.vscode/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, keep this in your local ignore config.
Internal Release Notes
This PR does a few things
count
andordinal
for i18nt
function. This does not add support onTrans
components because there wasn't a good example on what properties to set to support defaults***_one
derived keys. These were being set as "not derived" and given specific logic to populate them with default values. This doesn't make a lot of sense since we generally will want to add values to all derived keys or none of them.key
instead ofcleanKey
when setting the default value. This means that you'll end up with keys/value mappinginstead of
I can't think of any case where I'd want to map the key to value like this. It's better to use the
key
as the fallback because if you havekeyAsDefaultValue
set, you want the keys to actually work as translations.Validation Steps
yarn run test