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

chore(i18n): Introduce Locale, Message, and MessageBundle #8028

Merged
merged 1 commit into from Mar 23, 2015

Conversation

ewinslow
Copy link
Contributor

@ewinslow ewinslow commented Mar 2, 2015

These concepts will help us clean up the i18n code significantly.

TODO:

  • Pull the locale mapping into MessageBundle so that it is essentially Map<Locale, Map<string, Message>>
  • Add a __toString handler on Message that outputs the template. This would allow us to avoid calling "format" with no arguments, making views a wee bit more terse: I.e., {$message} instead of {$message->format()}
  • Rename Message to MessageTemplate
  • Make template protected instead of private
  • Replace isset checks with plain boolean checks where possible
  • Add support for translation_key_exists

$this->markTestIncomplete();
}

public function testDoesNotPerformSprintfFormattingIfArgsNotProvided() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test should give some idea of how I envision this all working together.

Eventually we will move most of the logic for loading the translation resources into a FilesystemMessageBundle class.

@juho-jaakkola
Copy link
Member

The approach LGTM

@ewinslow ewinslow force-pushed the i18n-oo branch 2 times, most recently from 62118c0 to ed8b447 Compare March 5, 2015 00:31
@ewinslow
Copy link
Contributor Author

ewinslow commented Mar 5, 2015

This should be ready for a final review before I pull it in. But if I don't hear back soon I'll just pull it in anyways as it is a private refactor that only adds classes/tests and doesn't change any runtime configuration/logic at all yet, plus I've already got an LGTM on the direction.

*
* @access private
*/
abstract class Message {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be a Template or MessageTemplate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, message template would be clearer.

@mrclay
Copy link
Member

mrclay commented Mar 5, 2015

My only concern here is how many objects we'd create/destroy translating a single page. Each translation constructs a new "en" Locale (that's easily optimized) but also a new SprintfMessage.

@ewinslow
Copy link
Contributor Author

ewinslow commented Mar 6, 2015

This definitely assumes object creation is cheap, and is designed this way to allow mixing sprintf templates with ICU templates. We could optimize in 2.0 by removing the message objects entirely since we will only want to support ICU.

@ewinslow
Copy link
Contributor Author

ewinslow commented Mar 6, 2015

But at the end of the day there is definitely the possibility of adding a few ms power request because of this change...

@jdalsem
Copy link
Member

jdalsem commented Mar 10, 2015

There are some recent additions made to the translation class (like translation_key_exists). Those features are not yet added to these classes. Will you add those later?

@ewinslow
Copy link
Contributor Author

I need to update this PR with those improvements.

@ewinslow
Copy link
Contributor Author

So the MessageBundle I think accomplishes what translation_key_exists wants, because you get just check if the return value of get is null. to see whether the key exists. No warnings are logged for trying to get a null value. Those are/should-be done at the translator level.

@juho-jaakkola
Copy link
Member

So this doesn't need anything else but a final review?

These concepts will help us clean up the i18n code significantly.
@ewinslow
Copy link
Contributor Author

That is correct

@ewinslow
Copy link
Contributor Author

Just posted several updates addressing all outstanding comments/todos, so PTAL and I will pull in after a couple LGTMs.

Main thing to note here is that we are just introducing the classes, not even using them yet.

@juho-jaakkola
Copy link
Member

LGTM

ewinslow added a commit that referenced this pull request Mar 23, 2015
chore(i18n): Introduce Locale, Message, and MessageBundle
@ewinslow ewinslow merged commit e164333 into Elgg:1.x Mar 23, 2015
@ewinslow ewinslow deleted the i18n-oo branch March 23, 2015 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants