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

Implement Cro::WebApp::I18N #42

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vendethiel
Copy link
Member

No description provided.

@japhb
Copy link

japhb commented Jun 3, 2021

Could you include some docs in the PR? It's unclear to me how a user of my service would specify their language and locality preference order (such as pt_BR, pt, en), and how those would be used at runtime. It's also unclear how this will work for non-LTR languages, or for mixing LTR and non-LTR info.


is get-response(route {
# XXX We currently fuzzy-match `en` and `en-XX`, should we really?
Copy link
Member

Choose a reason for hiding this comment

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

Is their precedent for doing this elsewehre?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so

Comment on lines +74 to +76
# TODO q sort
# TODO move this to a request method
$header.split(',')>>.trim.map(*.split(';')[0].trim)
Copy link
Member

Choose a reason for hiding this comment

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

Or at least Cro::HTTP::Request should have something like quality-header that does this

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, are other headers parsed this way?

Copy link
Member

Choose a reason for hiding this comment

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

I believe all the Accept* headers can use this.

load-translation-file('main', 't/resources/main.po', :language<en en-GB en-US>);
load-translation-file('main', 't/resources/main-fr.po', :language<fr fr-FR fr-CH>);
_-prefix 'main';
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this looks delightfully odd. I'm tempted to suggest translation-prefix, but then it's maybe less clear that it's about the _ function...perhaps it's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely To Be Nitpicked™.

@@ -747,6 +761,9 @@ role Cro::WebApp::Form {
method add-validation-error($message --> Nil) {
$!validation-state.add-custom-error($message);
}

#| Defines the prefix to use for translations using this form
method i18n-prefix(--> Str) { Str:U }
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this shoulda been done as a trait on the form class for consistency with it being an attribute trait on the fields.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered it as well, I can change it.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@Altai-man Altai-man left a comment

Choose a reason for hiding this comment

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

The POFile module is used, but is not specified in dependencies? Also new module files are introduced, but not added to META6.json, this will lead to test failures if tested with zef test ., which is what we must pass.


#| Load a translation file and store it with a given prefix and a given (set of) language
sub load-translation-file(Str:D $prefix, $file, :language(:languages(@languages))) is export {
my $pofile = POFile.load($file);
Copy link
Member

Choose a reason for hiding this comment

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

This probably wants some defensive CATCH.

}

sub match-language(Str @languages, Str $accept --> Int) {
if +@languages && $accept.defined {
Copy link
Member

Choose a reason for hiding this comment

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

Is taking the length here really necessary? Empty positionals are boolified as False.

try { request.annotations<language> } // Str
}

# TODO move this to Request
Copy link
Member

Choose a reason for hiding this comment

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

Either do or remove the TODO?

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.

4 participants