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

add(getString): add third argument to getString for fallback message #147

Merged

Conversation

hkasemir
Copy link
Contributor

@hkasemir hkasemir commented Feb 6, 2018

addresses issue #145
adds a third argument to the getString function in withLocalized wrapped components to allow for definition of a fallback message in case the message id is not fount in the message context.

Additional future benefit to be including fallback string in extraction scripts.

@hkasemir hkasemir force-pushed the hkasemir/add-getString-fallback branch from 50b16b7 to 402d83c Compare February 6, 2018 20:22
@hkasemir hkasemir force-pushed the hkasemir/add-getString-fallback branch from 402d83c to 47a773d Compare February 6, 2018 20:25
@hkasemir
Copy link
Contributor Author

hkasemir commented Feb 7, 2018

@stasm - I was sort of wondering if there would be a way to format the fallback string, but it didn't look like there was a good place to put it in there. would that be possible, or more trouble than it's worth?

@stasm
Copy link
Contributor

stasm commented Feb 8, 2018

We'd need to parse the fallback string to be able to format it. It's possible (although not trivial), but I don't think we should do it. It would quickly teach developers that they don't need to maintain the en-US file at all :)

Copy link
Contributor

@stasm stasm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @hkasemir, LGTM.

I was wondering if it would make sense to use a decorator instead of the third argument, but I don't have anything concrete to suggest. I think that the third argument is a good and simple way forward.

Also, what's up with add(getString) in the commit message?

@hkasemir
Copy link
Contributor Author

hkasemir commented Feb 9, 2018

oh, the commit message is following a convention I sometimes use at work that follows:

change_type(scope): commit message - if it's confusing I can edit the title

@stasm
Copy link
Contributor

stasm commented Feb 9, 2018

It was confusing to me at first :) No worries though.

@stasm stasm merged commit 5ebc8e2 into projectfluent:master Feb 9, 2018
@stasm
Copy link
Contributor

stasm commented Feb 9, 2018

Thanks again, @hkasemir!

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

Successfully merging this pull request may close these issues.

2 participants