-
Notifications
You must be signed in to change notification settings - Fork 6
Add localization support #10
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: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Tin Švagelj <[email protected]>
Signed-off-by: Tin Švagelj <[email protected]>
Signed-off-by: Tin Švagelj <[email protected]>
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!! It looks great. I like the localized keyword syntax. I think it can be refined a bit further. I have left some starting comments if you are willing to adress them.
Thank you once again
algorithmic.typ
Outdated
})) | ||
}) | ||
#let l(kw) = context { | ||
let lang = text.lang |
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.
I think it's better if the default lang stays "en". We can add a lang parameter to specify.
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 is to avoid breaking old documents?
I'd prefer a version bump because specifying lang
for every algorithmic
block feels off given that there's #set text(lang: "en")
which is per-documentation meant to be used for "[...] all other things which are language-aware".
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.
I gave it a try, it's a breaking change anyway because of:
#let Terminate(lang: none) = (smallcaps(localize("terminate", lang: lang)),)
#let Break(lang: none) = (smallcaps(localize("break", lang: lang)),)
which were previously values instead of functions.
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 is to avoid breaking old documents?
Yes, for now let's avoid breaking the default behaviour. We can keep everything to english keywords without respecting text.lang. For now, this PR is going to be merged as a patch release.
I'd prefer a version bump because specifying lang for every algorithmic block feels off given that there's #set text(lang: "en") which is per-documentation meant to be used for "[...] all other things which are language-aware".
Let's aim that for 2.0.0. I think it's best if we have a larger language support before a major release
I gave it a try, it's a breaking change anyway because of:
As for Terminate and Break, it's fine because that should return the same result.
algorithmic.typ
Outdated
let lang = text.lang | ||
|
||
if type(kw) == str { | ||
let kws = locale.at(lang, default: locale.at("en")) |
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.
In which case, a default here is not necessary. It's better to throw an error if the corresponding lang does not exist.
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.
Localization always falls back to default text. Using english isn't seen as wrong in most cases because even in LaTeX customizing algorithmx isn't all that common.
If we assume explicit lang
argument approach, I'd like to be able to specify a language even if it isn't supported yet and for the document to compile. If the text isn't localized, then I'd switch to "en" if I don't want it to change in future, or keep "de" if I want the proper locales to be used once they're supported.
With using the current text.lang
this is obviously the only reasonable approach.
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.
If the text isn't localized, then I'd switch to "en"
I'd rather algorithmic panic on that point. This is unexpected behaviour (and implicit) to fallback to english keywords if the lang input is not supported yet. Hence, removing the default from at. Don't bother with making it a nice panic, I can make that later.
- Rename `l` function to `localize`. Signed-off-by: Tin Švagelj <[email protected]>
Signed-off-by: Tin Švagelj <[email protected]>
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.
Thanks for the edits. This is looking well. Can you try to run the tests? I believe lang should be "en" by default, not none.
algorithmic.typ
Outdated
@@ -53,34 +78,38 @@ | |||
..table_bits | |||
) | |||
} | |||
#let algorithm-figure(title, supplement: "Algorithm", inset: 0.2em, ..bits) = { | |||
#let algorithm-figure(title, supplement: none, inset: 0.2em, lang: none, ..bits) = { |
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.
supplement should be "Algorithm" by default, not none.
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.
none
means none is specified explicitly, so it'll use default localization. This way you can still specify something like "Derp" without the rest of the code trying to localize that.
In docs you'd state the default is "Algorithm" as you did previously.
EDIT: Right, with keeping "en" the default, doing this doesn't make the API nicer. I'll make a commit addressing this review, as it's isolated to these changes it should be straightforward enough to simply revert it for 2.0.0.
algorithmic.typ
Outdated
})) | ||
}) | ||
#let l(kw) = context { | ||
let lang = text.lang |
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 is to avoid breaking old documents?
Yes, for now let's avoid breaking the default behaviour. We can keep everything to english keywords without respecting text.lang. For now, this PR is going to be merged as a patch release.
I'd prefer a version bump because specifying lang for every algorithmic block feels off given that there's #set text(lang: "en") which is per-documentation meant to be used for "[...] all other things which are language-aware".
Let's aim that for 2.0.0. I think it's best if we have a larger language support before a major release
I gave it a try, it's a breaking change anyway because of:
As for Terminate and Break, it's fine because that should return the same result.
algorithmic.typ
Outdated
let lang = text.lang | ||
|
||
if type(kw) == str { | ||
let kws = locale.at(lang, default: locale.at("en")) |
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.
If the text isn't localized, then I'd switch to "en"
I'd rather algorithmic panic on that point. This is unexpected behaviour (and implicit) to fallback to english keywords if the lang input is not supported yet. Hence, removing the default from at. Don't bother with making it a nice panic, I can make that later.
Made "en" the default locale and `localize` panic on missing locale as requested. Signed-off-by: Tin Švagelj <[email protected]>
Done |
Signed-off-by: Tin Švagelj <[email protected]>
@Caellian, thank you for the modifications. I have yet to look at them, but I will definitely find the time in the coming weeks. Thank you again! |
@Caellian I checked out your branch and tried to run the tests, but they don't run. Please test your PR so that I can review it! Thank you. |
Makes algorithmic read
text.lang
and use appropriate keywords/control words for the currently used langauge. Idea is that contributors could add their languages over time as they need them (withen
being fallback).If the document is multilingual this makes it very straightforward to use appropriate language based on current context.
I also added support for syntax like:
Locales besides
en
andhr
were generated using an LLM as placeholder values. If locale is not in the file, or an empty string for somekw
, then english is used as fallback. For functions it's english, then first value, andnone
if empty dictionary was provided.