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

Internationalization #14

Merged
merged 13 commits into from
Sep 6, 2022
Merged

Internationalization #14

merged 13 commits into from
Sep 6, 2022

Conversation

jonschumacher
Copy link
Contributor

Closes #13

src/computational_thinking.jl Outdated Show resolved Hide resolved
src/i18n.jl Outdated Show resolved Hide resolved
src/computational_thinking.jl Outdated Show resolved Hide resolved
src/i18n.jl Outdated Show resolved Hide resolved
src/i18n.jl Outdated
present_str(lang::Lang) where {Lang <: English} = "present"
present_mode_str(lang::Lang) where {Lang <: English} = "Present Mode"

default_language = Ref{AbstractLanguage}(EnglishUS())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a frequent user of references in Julia. Would you mind explaining why you used one here?

Copy link
Contributor Author

@jonschumacher jonschumacher Sep 2, 2022

Choose a reason for hiding this comment

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

This was mainly based on this comment. When looking deeper into this, I could not find details on this. I am also not sure how much penalty we get by having this variable type Any. With Julia 1.8, we could also directly annotate the type to avoid the instability but this would break compatibility with older Julia versions.

src/PlutoTeachingTools.jl Outdated Show resolved Hide resolved
src/i18n.jl Outdated Show resolved Hide resolved
@eford
Copy link
Collaborator

eford commented Sep 2, 2022

Thanks. I skimmed it and think this looks very promising. I asked some questions where I wasn't sure about the pros/cons of the different ways we could implement something.

In order to minimize the risk of breaking changes in the future, I think it would be good to have at least one other language implemented before we merge the changes. That way we'll be more likely to notice any complications that we should address now.

@eford eford added the enhancement New feature or request label Sep 2, 2022
@jonschumacher jonschumacher marked this pull request as ready for review September 2, 2022 07:39
@jonschumacher
Copy link
Contributor Author

Thanks for the quick review. The draft was mainly meant to inform you where I am doing stuff, but I think I now addressed all the issues. The tests are running through and after setting the language, I get the correct translations. From my perspective we can merge. What do you think?

@eford
Copy link
Collaborator

eford commented Sep 3, 2022

Thanks! I tried using it with one of the notebooks for my current class. I got several error messages like

correct()@computational_thinking.jl:137
top-level scope@[Local: 1](http://localhost:1234/edit?id=efd44382-2b35-11ed-397b-4188d5f532fa#)[inlined]

So far, all the problems I noticed could be traced to a function where a (currently) required argument has a default value. In the PR, the default value is set by a function call that depends on lang, but lang is only set in a subsequent argument. For example

correct(text=rand(yays(lang)), lang::AbstractLanguage = default_language[]) = Markdown.MD(Markdown.Admonition("correct", correct_str(lang), [text]));

The other cases I noticed were protip, still_missing, still_nothing and keep_working.
I see two potential solutions:

  1. Make any optional parameter a named parameter
correct(; lang::AbstractLanguage = default_language[], text=rand(yays(lang)) ) = Markdown.MD(Markdown.Admonition("correct", correct_str(lang), [text]));

Pros:

  • Seems workable and clean
  • Would make it easy to generalize further if we wanted to allow people to override the hint_str, tip_str, etc. manually. (But If people override the default hint_str, tip_str, etc. it won't be easy to swap out languages. So maybe its better to not to generalize functions like hint and tip.)

Cons:

  • This would be a breaking change to the API. (Ok with me in this case.)
  • People who manually set lang would have to write lang= over and over again. (Maybe not an issue if we think people will just use set_language! and not specify it manually.)

Open Question:

  • Would we make lang a named argument for every function for consistency, even though most functions don't need it that way? If not, people would need to know which functions have named parameters.
  1. For the functions that currently have arguments with default values (other than lang), make two versions (one with all named arguments, one with unnamed arguments), so as to preserve backward compatibility and prevent people from having to use named arguments.
correct(; lang::AbstractLanguage = default_language[], text=rand(yays(lang)) ) = Markdown.MD(Markdown.Admonition("correct", correct_str(lang), [text]));
correct(text, lang::AbstractLanguage = default_language[]) = correct(;lang=lang, text=text)

Pros:

  • Makes changes non-breaking to API
  • Could do this just for the ~5 functions that currently need it, and still have option of doing something similar for other functions down the road.

Cons:

  • A few more lines of code to maintain

Currently, I'm leaning towards option #2.
Any thoughts?

@jonschumacher
Copy link
Contributor Author

jonschumacher commented Sep 6, 2022

I now opted for option 2 and implemented it. Those few more lines should not be hard to maintain. I now also added a separation between formal and colloquial German.

Copy link
Collaborator

@eford eford left a comment

Choose a reason for hiding this comment

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

Thanks

@eford eford merged commit c83f596 into JuliaPluto:main Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internationalization
2 participants