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

Resolve rework requests #4

Merged
merged 10 commits into from
Oct 11, 2022
Merged

Resolve rework requests #4

merged 10 commits into from
Oct 11, 2022

Conversation

LeoniePhiline
Copy link

@LeoniePhiline LeoniePhiline commented Oct 10, 2022

Resolve some rework requests at djc#700

fix: Remove commented out i18n tests

Fixes thread https://github.com/djc/askama/pull/700/files#r934558288

fix: Add trailing newline to i18n test templates

Fixes thread https://github.com/djc/askama/pull/700/files#r934508651

fix: cargo fmt, remove trailing whitespace

Fixes CI lint at https://github.com/djc/askama/pull/700/checks

fix: Rename feature localization to i18n

Fixes thread https://github.com/djc/askama/pull/700/files#r934510739

opinionated: Rename initialization macro localization! to i18n_load!

Rationale: Match module name and renamed feature name, while still making sense to human readers.

style: Change wording "have to" to "need to"

Improve message wording.

refactor: Confine i18n code inside i18n modules

You can now call

askama::i18n::load!(LOCALES);

And i18n features no longer leak out of the i18n modules.

fix: Hide dependency fluent_templates from library users

Just a fix, reverting dependency management to a better state from before the refactor. Library users should not need to add fluent_templates as their crate's dependency.

fix(deps): Update dependency fluent-templates to 0.8.0

Update fluent-templates to the latest version. The lookup function was changed to return an Option<String>.
I chose to unwrap() the returned Option in the code generator, because the key being looked up (message) is checked at compile time.

Dependency changes: XAMPPRocky/fluent-templates@v0.7.1...v0.8.0

docs: Add initial i18n module documentation

Adds some initial documentation, based on a test case (simplified).

@89Q12
Copy link
Owner

89Q12 commented Oct 10, 2022

Sick, thanks for helping out^^
Looks good to me so far and the rename of the macro, makes sense to me too!

@LeoniePhiline
Copy link
Author

I do think renaming the macro makes sense, but I am not sure if i18n_load! is the correct name.

Here's my thoughts:

The _load! part seems useful to understand the verb of the macro's actions.

However, I've been pondering about localization vs internationalization; e.g. if the {{ localize(...) }} expression should be renamed or not.

Reading about localization vs internationalization, I found that internationalization is the right name for the framework that enables localization. To me this means that it will not make sense to replace all mentions of localization, localize or localizer to variants of i18n.

The rationale being: "Internationalization" widens the space or scope to all locales, while "localization" then, as a second step, deliberately reduces the space or scope down to one single desired locale.

I've been considering to call the macro load_locales!, but that duplicates "locales" with the passed token (load_locales!(LOCALES) -> twice "locales").

What do you think? Is i18n_load! a good name? What would be a possible better name, if any?

@LeoniePhiline
Copy link
Author

LeoniePhiline commented Oct 10, 2022

I'm ok with merging here. Other changes can be done in a separate PR.


Continued discussion:

About https://github.com/djc/askama/pull/700/files#r934508219
What do you think - should this macro be removed, and the macro it matches on be called directly?

This would also solve https://github.com/djc/askama/pull/700/files#r991425622

@89Q12
Copy link
Owner

89Q12 commented Oct 10, 2022

Regarding the name i18n_load!its fine but I think load_locales! is great too but as you already pointed it doubes locales. What I thought of is maybe load_i18n!. What do you think?

I personally would not rename the expression {{ localize(...) }}, once for the reason you already pointed out but also because other libs do this too.

Also removing the macro hm i don't know in my opinion its better to a nice friendly error message there but I think upstream should decide on wether to remove it or not. At least that are my thoughts on it.

I will merge this once I finished eating/ the current episode :>)

@LeoniePhiline
Copy link
Author

LeoniePhiline commented Oct 10, 2022

What I thought of is maybe load_i18n!. What do you think?

I would strongly prefer i18n_load!, as the macro in its current form is not namespaced. It is used directly as askama::...!().
Therefore, mentioning the scope i18n before the action load reduces mental load when reading the code.

We could also drop the crate-level macro and just rename the (currently misnamed?) i18n::derive macro to load.
Library users would then invoke:

askama::i18n::load!(LOCALES);

or:

use askama::i18n;

i18n::load!(LOCALES);

If a user missed enabling the feature then the compiler would inform that the i18n module is gated by the i18n feature flag.

@89Q12
Copy link
Owner

89Q12 commented Oct 10, 2022

Sounds good to me, I more of a "proxy" anyways with this PR.
But yeah I definitely prefer moving it into i18n and renaming the i18n::derive function.
In case you wanna read code that uses the i18n feature

@LeoniePhiline
Copy link
Author

But yeah I definitely prefer moving it into i18n and renaming the i18n::derive function.

Won't work, unfortunately, due to proc-macro restrictions:
https://stackoverflow.com/questions/56713877/why-do-proc-macros-have-to-be-defined-in-proc-macro-crate

image

@89Q12
Copy link
Owner

89Q12 commented Oct 10, 2022

TIL, then lets change it to i18n_load! but again thanks for looking into it!

Proc-macro askama_derive::i18n_load!() must remain in root namespace,
due to restrictions in proc-macro exports.
However, it is re-published as `askama::i18n::load!()`.
@LeoniePhiline
Copy link
Author

Fixed it. Check original post for notes for each added commit.

@LeoniePhiline
Copy link
Author

Added a bit of documentation.

image

@89Q12 89Q12 merged commit 7f82494 into 89Q12:i18n Oct 11, 2022
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