google / mdbook-i18n-helpers

Translation support for mdbook. The plugins here give you a structured way to maintain a translated book.
Apache License 2.0
127 stars 25 forks source link

Build translations from mdbook #84

Open sakex opened 11 months ago

sakex commented 11 months ago

Fixes #13. Fixes #18.

sakex commented 11 months ago

We should bump the version as alpha in cargo.toml and publish the new version

mgeisler commented 11 months ago

This basically moves the for loop from publish.yml to the Rust code?

Later I guess this for-loop-renderer (🙂) will know to pass in i18n-releated information to the book — or maybe not since the other renderer will already be able to grab this from the book.toml file?

sakex commented 11 months ago

This basically moves the for loop from publish.yml to the Rust code?

Yes that's the only thing it does. The advantage is that we get a better developer experience for translators. They see all translations at the same time without having to rebuild multiple times with a different command. Live reload is also supported. It's a small win but I think it's worth it.

mgeisler commented 11 months ago

A question: I imagine that we would want a book.toml that looks like this:

[output.i18n]

This enabled the i18n backend and disables the html backend (as per the documentation).

I imagine most projects will enable just this one backend and thus get the generated output put into book/ by default.

In that setup, what does the translate_all_languages setting do? When translate_all_languages is off, do I get the same output into book/ as if I had [output.html] enabled instead? If translate_all_languages is on, then I get book/ as before plus book/xx/ for each locale?

I would like to see this described somewhere. Probably in the docstring for the new binary at first — later we can move this to a README for the new crate so that we can publish it on crates.io.

sakex commented 11 months ago

A question: I imagine that we would want a book.toml that looks like this:

[output.i18n]

This enabled the i18n backend and disables the html backend (as per the documentation).

I imagine most projects will enable just this one backend and thus get the generated output put into book/ by default.

In that setup, what does the translate_all_languages setting do? When translate_all_languages is off, do I get the same output into book/ as if I had [output.html] enabled instead? If translate_all_languages is on, then I get book/ as before plus book/xx/ for each locale?

I would like to see this described somewhere. Probably in the docstring for the new binary at first — later we can move this to a README for the new crate so that we can publish it on crates.io.

We do need to have the HTML renderer to generate HTML output. It is also documented that we can simply set [output.html] in book.toml. We should mention that in the doc. They mention it in mdbook-pdf for instance.

Since the preprocessor only edits the content of the book object but doesn't create any files itself, it will work with any other renderer as well, which makes me think that we could change the parameter from move_translations_to_html_directory: bool to move_translations_to_renderer_outputs: Vec<String> which would take arguments like html and move the translated output from $lang/$renderer/ to $renderer/$lang

mgeisler commented 11 months ago

We do need to have the HTML renderer to generate HTML output. It is also documented that we can simply set [output.html] in book.toml. We should mention that in the doc. They mention it in mdbook-pdf for instance.

Ah, I guess that makes sense: people should configure the HTML output via the normal html renderer settings! I was thinking we would remove this, but it's not a good idea.

Since the preprocessor only edits the content of the book object but doesn't create any files itself, it will work with any other renderer as well, which makes me think that we could change the parameter from move_translations_to_html_directory: bool to move_translations_to_renderer_outputs: Vec<String> which would take arguments like html and move the translated output from $lang/$renderer/ to $renderer/$lang

I like the idea of making this generic over the kind of renderer! As a litmus test, you could ensure that it's possible to generate i18n output for the built-in markdown renderer.

Now that discuss this: I think the correct strategy would be to copy the output from the other renderer and into the i18n renderer's output directory. This avoids writing anywhere except for ctx.destination and it is thus consistent with how mdbook renderers are supposed to work.

From skimming Alternative Backends, I don't see any ordering between the renderers. Do we rely on the existing HTML output being present? If we do, we should probably not do this and generate it ourselves :slightly_smiling_face:

sakex commented 11 months ago

From skimming Alternative Backends, I don't see any ordering between the renderers. Do we rely on the existing HTML output being present? If we do, we should probably not do this and generate it ourselves 🙂

In this renderer, we don't care about what runs first. For the new template renderer, we will need html to be present. The rendering order is based on the order in which they are defined in book.toml. I remember seeing it in the doc, but I can't find it anymore, reading the code, it is how it is handled.

mgeisler commented 11 months ago

Just a note about Merge branch 'main' into translate-from-mdbook. Please rebase your commits on top of main instead of merging main into the branch. By rebasing, you create a clean and useful history — by merging, you end up with a mix of your changes and other changes and it's hard to read the history afterwards.

For this reason, I will want to squash merge the PR if it has merge commits.

mgeisler commented 11 months ago

From skimming Alternative Backends, I don't see any ordering between the renderers. Do we rely on the existing HTML output being present? If we do, we should probably not do this and generate it ourselves 🙂

In this renderer, we don't care about what runs first.

We don't care because we run the html renderer ourselves?

For the new template renderer, we will need html to be present.

Could you instead run the html renderer yourself? Output the files to your ctx.destination and the process each of them through the template engine.

The rendering order is based on the order in which they are defined in book.toml. I remember seeing it in the doc, but I can't find it anymore, reading the code, it is how it is handled.

Okay, thanks for checking! If it isn't documented, then we should not rely on it. If the order is important, could you then open an PR to the upstream repository to define the order in the documentation?

sakex commented 11 months ago

Just a note about Merge branch 'main' into translate-from-mdbook. Please rebase your commits on top of main instead of merging main into the branch. By rebasing, you create a clean and useful history — by merging, you end up with a mix of your changes and other changes and it's hard to read the history afterwards.

For this reason, I will want to squash merge the PR if it has merge commits.

Yes, I did that because it is more convenient. I only want one commit for this PR anyway, so squashing is expected

sakex commented 11 months ago

Hey @mgeisler . Is it Ok to merge now (with squash) so that I can use this as a base for the other renderer? After the renderer is done, I'll make a test book to test all the features

mgeisler commented 11 months ago

Hey @mgeisler . Is it Ok to merge now (with squash) so that I can use this as a base for the other renderer?

I think we need some documentation before we merge this. It's not clear to me what the expectations are for the user.

As an example, I believe you said that the html backend has to be enabled?

After the renderer is done, I'll make a test book to test all the features

Please add a test here already, otherwise it's hard to know what to expect. Also, update the first PR comment above to explain what this does (hint: reuse that text for your module docstring and for the README and USAGE).

mgeisler commented 11 months ago

From skimming Alternative Backends, I don't see any ordering between the renderers. Do we rely on the existing HTML output being present? If we do, we should probably not do this and generate it ourselves 🙂

In this renderer, we don't care about what runs first.

We don't care because we run the html renderer ourselves?

We still need to answer this essential question: how can we rely on ctx.destination + "/../html" existing when this backend is executed? I believe we still have that implicit expectation?

sakex commented 11 months ago

From skimming Alternative Backends, I don't see any ordering between the renderers. Do we rely on the existing HTML output being present? If we do, we should probably not do this and generate it ourselves 🙂

In this renderer, we don't care about what runs first.

We don't care because we run the html renderer ourselves?

We still need to answer this essential question: how can we rely on ctx.destination + "/../html" existing when this backend is executed? I believe we still have that implicit expectation?

No, it's not required anymore

simonsan commented 11 months ago

I sometimes open this PR to look at the current state, and I wanted to let a compliment here for you both. I really like your interactions and styles of giving feedback. May this become a great feature :) Much of love! 👍🏽