search

ultrabug / mkdocs-static-i18n

MkDocs i18n plugin using static translation markdown files
https://ultrabug.github.io/mkdocs-static-i18n/
MIT License
239 stars 38 forks source link

Refactor plugin #216

Closed ultrabug closed 1 year ago

ultrabug commented 1 year ago

This is a major rewrite of the plugin logic with breaking changes :warning:

Core rationale

I felt the need to make this plugin simpler and leverage on mkdocs.command.build to effectively build each language one after the other.

Benefits:

Configuration rationale

The other thing I saw with the adoption of this plugin is that users could benefit from a more generic way of overriding parts of their mkdocs.yaml for each language.

I have removed a bunch of configuration options and allowed each language configuration to dynamically override parts of the mkdocs.yaml file.

Users can do things like this for instance without the need to maintain multiple mkdocs.yml:

theme:
  name: material
  font: false
  icon:
    logo: material/file-document-multiple
  palette:
    primary: blue

plugins:
  - search
  - i18n:
      docs_structure: suffix
      languages:
        - locale: en
          default: true
          name: English
          build: true
          site_name: "MkDocs static i18n plugin demo (en)"
        - locale: fr
          name: Français
          build: true
          site_name: "Démo du plugin MkDocs static i18n (fr)"
          theme:
            palette:
              primary: red
          nav_translations:
            Topic1: Sujet1 Traduit
            Topic2: Sujet2 Traduit
        - locale: de
          name: German
          build: true
          site_name: "Demo des plugin MkDocs static i18n (de)"
          theme:
            palette:
              primary: yellow
          nav:
            - Home: index.md
            - Das Topic 2: topic2/README.md

One can now override the color of the theme per language, its navigation, whatever!...

Breaking changes

This comes with a more opinionated way of building languages, especially the default language and stricter configuration options.

Enhancements

I expect the build time to be faster than before since we're not (deep)copying objects anymore and we iterate over file objects less.

ultrabug commented 1 year ago

@kamilkrzyskow Here it is as promised, the major rewrite of the plugin!

I tried to be clear on the PR comments.

This is still WIP because

I'd like your opinion if possible :+1:

ultrabug commented 1 year ago

Thanks a lot for your valuable and most appreciated thoughts @kamilkrzyskow, I'd like to take this chance to thank you for the time and goodwill you put to improving this plugin :heart:

Indeed I promised I'd make a PR early out of respect of all your contributions. This is clearly a WIP and I don't pretend it's 100% operational as you noted.

Even more importantly, I absolutely have no intention of merging this in place of anyone's contributions. Being a long time OSS author and contributor, I'm also very strict about that.

I'll address your points by working on tests so that we can start seeing something that actually validates things in a proper manner.

Thanks again!

kamilkrzyskow commented 1 year ago

Oh gosh, I didn't want to belittle your work or accuse you @ultrabug of anything bad 👀 💦 I just given my feedback, pointed out some issues, and your response comes of as sort of "apologetic" with the amount of explanations and "thank yous", but then again I might just be missing the tone as I'm not native English.

You don't owe me any thanks, I'm helping here out, because I'm using your plugin ✌️, so thanks for making it ❤️ I didn't want to manage multiple mkdocs.yml files and manage merging together multiple builds myself, therefore I'll be here until the effort of adding light adjustments to the plugin will not exceed the effort of having to setup and maintain a separate build solution 😺

I will look into why my hook doesn't work, and investigate the missing event calls later today. Maybe before writing tests you should fix the obviously broken nav with .md urls😉, unless you prefer the Tests-First TDD approach.

Also I've remembered another idea I've had. About separating the search_index.json file for each language, tbh I didn't even check if it works currently, since the nav didn't work. For context, some time ago, I've created a hook-based solution for dynamic theme overrides: https://github.com/squidfunk/mkdocs-material/discussions/5060

Side-note: I'm currently, slowly, finishing a refactor there to remove redundancy, and will update the guide soon™️

Since the sitemap / search_index paths are hard-coded in MkDocs themselves, and even the Material theme author doesn't think it's a good idea to give plugin-level access to modifying that path https://github.com/ultrabug/mkdocs-static-i18n/issues/196#issuecomment-1448755715 , I think you could incorporate a similar solution as my theme override method to change the internal files on the fly to change those hard-coded paths. This would give the option to separate the search_index file across language versions, and decrease the file size for each language.

Currently, our site uses a JavaScript solution to further remove any duplicated search links / preserve the currently selected language. If the idea above won't work out you could insert the extra_javascript to handle the duplicates.

kamilkrzyskow commented 1 year ago

I finally got around to checking the issue with my hook. After looking at some debug prints, it turned out my hook was partially the problem. All of the events are running properly in the current setup.

The issue was that my hook created a config["extra"]["custom_dict_mapping"] entry in the on_config event, and other parts of the code checked for changes in that custom_dict_mapping aka if it exists or contains a specific value. I never cleared/deleted that entry, because Python figured it out at the end of the program before. However, now after a on_post_build an additional on_config etc. another build event sequence is being run.

https://github.com/ultrabug/mkdocs-static-i18n/blob/9e4700d2037c80ccd9461663328374505049e198/mkdocs_static_i18n/plugin.py#L202-L203

Perhaps it would be a good idea to reload the base mkdocs.yml config file and only after that change the language-related settings within. There might be more cases like mine that modify the config state and expect an unmodified state before 🤔

After your last commit with the suffix tests, the previous awesome-pages error was fixed 🎉, but it revealed another issue:

  File "C:\MyFiles\git\awesome-pages\mkdocs_awesome_pages_plugin\navigation.py", line 96, in <lambda>
    key = lambda i: get_title(i)
                    ^^^^^^^^^^^^
  File "C:\MyFiles\git\awesome-pages\mkdocs_awesome_pages_plugin\navigation.py", line 279, in get_title
    with open(item.file.abs_src_path, encoding="utf-8-sig", errors="strict") as f:
              ^^^^^^^^^
AttributeError: 'Link' object has no attribute 'file'. Did you mean: 'title'?

So this is the part of the code I added to handle titles, and this revealed a bug in my code solution, because Links shouldn't be processed as files. However, the bug here is that my docs don't have Link type nav items at all, yet it somehow served it back to awesome-pages 🤔 Or maybe I lack some understanding on the matter, at least this error didn't occur on the pre-refactor version.

Your refactor already found bugs in 2 of my solutions, thanks a lot 🤝 Keep it up ✌️

kamilkrzyskow commented 1 year ago

Happy Easter @ultrabug, how are things progressing? Do you have any uncommitted code locally, or did you not have the time to work on the refactor? I want to take a jab at making it functional, since I planned to add Social Cards support this month (aka fix paths to some images, and add site_description override support), and since any changes I make towards that will be obsolete after the refactor I might as well tinker with the refactor instead.

ultrabug commented 1 year ago

I've been OOO since last week, just getting back to business. Will read up and catch up @kamilkrzyskow please bear with me.

I'd like to get the standard tests working again, I catched a bug thanks to this.

External plugins do add complexity to the mix, I'd like the local tests to be OKAY before going further, sounds like a sane way forward.

If you have ideas on local tests that should be added without external plugins, I'd be happy to add them to harden the local tests first.

I target to add tests for those external plugins as well to make sure that we are compatible in the future.

ultrabug commented 1 year ago

I just pushed my local tests fixes so far

ultrabug commented 1 year ago

@kamilkrzyskow hi, I'm happy to have finished reworking the tests

Python 3.8 has a missing implem for a pathlib feature that I'll have to troubleshoot but the current work on tests allowed me to catch and fix a lot of things!

Can you give me your updated impressions please?

kamilkrzyskow commented 1 year ago

Hi @ultrabug, I'm sorry for my lack of contribution, I wanted to make the nav work, but then I wasn't sure about whether to process it recursively or iteratively and I was unsure of some other things too, so I decided to quietly wait till the weekend to work on it with a fresh start. 🤞 I hope I will be able to figure it out during the next 2 days. Since I already had a honest attempt at working with the code I will first talk about some things I noticed, which I felt are not optimal stylistically, or basically not as I like them, so take it with a grain of salt ^^. Apart of the black -l120 and type hints I mentioned previously:

As for the more general code "analysis":

kamilkrzyskow commented 1 year ago

Let me know if that's the feedback you were looking for or if I'm supposed to approach it differently 🙋

ultrabug commented 1 year ago

Thanks a lot @kamilkrzyskow ; I guess I'll work on addressing your comments first.

For now I was more interested in making sure the code is actually functional rather than style/optimization but you got a point in the sense that improving this will also improve readability of the review as well :+1:

ultrabug commented 1 year ago

Instead of calling the reconfigure functions via reconfigure.reconfigure_mkdocs_config(self, config) I would prefer a cleaner self.reconfigure_mkdocs_config(config) syntax, because there always should be only 1 instance of the plugin anyways. To be able to do that in the reconfigure module put every function in a ExtendedPlugin(BasePlugin) class, move the config_scheme and init there, and then create the I18n(ExtendedPlugin) class. I did a quick test and it would work. Some of those functions even use self instead of the i18n_plugin param variable, because it's natural wink

done

ultrabug commented 1 year ago

Every MkDocs event function should preserve the original function signature, for example keep config: MkDocsConfig instead of mkdocs_config: MkDocsConfig.

done

There are some cases where variables are reassigned to change their name, and I think it's unnecessary. While I understand that the i18n_ prefix is added to indicate the variable has been modified by the i18n plugin I feel like it's only adding confusion. I think that the i18n prefix should only be used by original plugin overrides (I18nFiles), but after adding type hints it will also be unnecessary.

done (maybe not everywhere yet)

ultrabug commented 1 year ago

I think that the I18n plugin class and its event functions should act as a sort of guide to the rest of the restructuring. For example in the on_config the conditional is checked inside of the event function and then the data is passed to the more specialized function heavy_check_mark :

done

ultrabug commented 1 year ago

Perhaps it would be a good idea to reload the base mkdocs.yml config file and only after that change the language-related settings within. There might be more cases like mine that modify the config state and expect an unmodified state before thinking

Reloading the config file would mean to do something like this:

reloaded_config = load_config(config.config_file_path)
reloaded_config.plugins = config.plugins
build(reloaded_config)

because we need to preserve the plugins' instances (search and i18n for instance need to survive build runs) so I don't know

I'm always doubtful about the fact that i18n should be the only plugin to make the efforts to be compatible with the others and adapt to whatever their authors decide to do :(

ultrabug commented 1 year ago

EDIT: Actually I just noticed that the pkq_resources approach was there before, I just didn't notice it facepalm. Also investigating it further. it has been recently deprecated too

migrated to importlib.metadata :+1:

ultrabug commented 1 year ago

I see that you reduced the amount of tests almost by half, and it's not because there are no folder structure tests. I'm assuming they will be added once the code works ^^, but felt weird that they got removed instead of commented out, for example the test_urls.py

I have never been happy about my tests and some of them were overlapping (test_urls) with the new structure. I thought that dropping them would clear the path to some improvements without having hundreds of comment lines floating around.

I'll check your PR and start working on improving the ones that I kept.

ultrabug commented 1 year ago

There is a lack of config override validation. MkDocs 1.4.0 introduced a Config subclassing to validate the options in a plugin. However, I'm not sure if it's possible to introduce it in the current code, because a given language key can be any valid locale value, but maybe there is a way to extend the MkDocsConfig class and validate the inner language overrides. This approach would also allow to avoid using brackets [] to access variables.

I also considered that @kamilkrzyskow but like you I struggled with the "locale is the key of the dict config" problem...

So instead, I switched to using a ListOfItems but this means that we have to get the languages more explicit with a locale key and now the configuration would look like this:

plugins:
  - search
  - i18n:
      docs_structure: suffix
      languages:
        - locale: en
          default: true
          name: English
          build: true
          site_name: "MkDocs static i18n plugin demo (en)"
        - locale: fr
          name: Français
          build: true
          site_name: "Démo du plugin MkDocs static i18n (fr)"
          theme:
            palette:
              primary: red
          nav_translations:
            Topic1: Sujet1 Traduit
            Topic2: Sujet2 Traduit
        - locale: de
          name: German
          build: true
          site_name: "Das plugin MkDocs static i18n (de)"
          theme:
            palette:
              primary: yellow
          nav:
            - Home: index.md
            - Das Topic 2: topic2/README.md

What I like with this approach is that we get strong validation ofc but I can also validate some other keys like the nav. It would look like this:

from mkdocs.config.base import Config
from mkdocs.config import config_options

class I18nPluginLanguage(Config):
    build = config_options.Type(bool, default=True)
    default = config_options.Type(bool, default=False)
    locale = config_options.Type(str)
    name = config_options.Type(str)
    nav = config_options.Optional(config_options.Nav())
    nav_translations = config_options.Optional(config_options.Type(dict))
    theme = config_options.Optional(config_options.Type(dict))

class I18nPluginConfig(Config):
    """
    config_scheme = (
        (
            "docs_structure",
            Choice(["folder", "suffix"], default="suffix", required=False),
        ),
        ("fallback_to_default", Type(bool, default=True, required=False)),
        ("languages", Locale(dict, required=True)),
        ("reconfigure_material", Type(bool, default=True, required=False)),
        ("reconfigure_search", Type(bool, default=True, required=False)),
    )

    """
    docs_structure = config_options.Choice(["folder", "suffix"], default="suffix")
    fallback_to_default = config_options.Type(bool, default=True)
    reconfigure_material = config_options.Type(bool, default=True)
    reconfigure_search = config_options.Type(bool, default=True)
    languages = config_options.ListOfItems(config_options.SubConfig(I18nPluginLanguage, validate=True))

If you agree with that proposal, I'll go on with updating the code base to use this new config scheme.

kamilkrzyskow commented 1 year ago

You're working ultra fast like always @ultrabug, while I got sick over the weekend and couldn't do any work 😞 At this point I'm beginning to think I'm slowing you down, since you're coding faster, than I snap my fingers. If you can wait a little bit longer, please give me some more time to fix the navigation 🙏

You introduced changes from my last post (thanks for approving 🙏), but I'm not sure if you've given me any definitive answer about using black with a bigger line length 100 or 120. Possibly running it with the --remove-magic-trailing-comma on a second refactor run could also remove some verticality.

[...] we need to preserve the plugins' instances (search and i18n for instance need to survive build runs) so I don't know I'm always doubtful about the fact that i18n should be the only plugin to make the efforts to be compatible with the others and adapt to whatever their authors decide to do :(

Search needs to be preserved, because it still has to accumulate all of the URLs in the main build, to have access to all of them and then the plugin remove duplicates, right? What about my suggestion of modifying the served JavaScript to load different search_indexes? It would remove the need of deleting duplicates. Each language would have their own search_index.

Also this plugin fights the "standard" of separating the config into different files. and separate builds. Even MkDocs itself recommends using different builds, because the search_index would bloat up (like it does currently). Most other plugins don't even look at i18n support, therefore it's understandable there are issues :((( To limit issues this plugin would have to become the new standard, and that's not so simple, because it has to support everything first, and this refactor tries to accomplish that.

The config change looks promising, but would require yet another test config reconfiguration, maybe there is another way to link the current Locale Type with a SubConfig validation, then again there are less tests now, so maybe the test refactor will be less painful. EDIT: If you asked me if I agree with the config structure, then I see no downside of it being a list instead of dict 👍 Did you try to extend the MkDocsConfig class instead of just Config class I18nPluginLanguage(MkDocsConfig):, then override site_name to not be required. I think it mentions that the order of properties matters, so if extending isn't an option maybe using a Mixin class is, so that the plugin-exclusive option would be loaded first? I didn't check the interpreter yet to see the order of attributes myself, but this change would give access to complete validation.

I appreciate your work and like always let me know if I did miss anything in my response 🙋

ultrabug commented 1 year ago

What about my suggestion of modifying the served JavaScript to load different search_indexes?

While the benefits sound cool, I'm not keen on having to keep up with material's and mkdocs' JavaScript code (which I also don't master very much and have poor interest in). So I'd rather keep with their implementation and simply "do my best".

kamilkrzyskow commented 1 year ago

@ultrabug My IDE reported that importlib.metadata isn't available in Python 3.7, and Python 3.7 is still supported by MkDocs Looking at the docs importlib.resources is available since 3.7 and importlib.metadata is available since 3.8.

ultrabug commented 1 year ago

I'm not sure if you've given me any definitive answer about using black with a bigger line length 100 or 120.

I'll switch to 100 once I'm done with the new config + tests update

ultrabug commented 1 year ago

If you asked me if I agree with the config structure, then I see no downside of it being a list instead of dict

done + tests OK

Did you try to extend the MkDocsConfig class instead of just Config class I18nPluginLanguage(MkDocsConfig):, then override site_name to not be required.

yep tried it, it's a pain, maybe I got it wrong... for now I'm satisfied with what I have

ultrabug commented 1 year ago

I switched to black 100 as suggested :+1:

ultrabug commented 1 year ago

Compat code for is_relative_to added... CI is now green for all versions!

kamilkrzyskow commented 1 year ago

@ultrabug I finally managed to fix navigation and some other stuff on my end. Currently there are no warnings in my docs. Not a lot of changes, but it's honest debugging work OK 😮‍💨 Worst part is that I thought the issue is in the on_nav and after some (not short) amount of time I noticed the issue has to be in the Navigation creation 🤦

I've opened a PR pointed at this branch, let me know what you think 🙋

Most issues that I have now are related to the build(config):

...and something else, but I forgot currently. Keeping this message a little bit shorter 🤝

kamilkrzyskow commented 1 year ago

I did a force push and noticed that the workflow file is probably configured incorrectly image 2 runs went off for the same commit

Much overdue EDIT: As the image shows both runs are from completely different times, and it didn't happen since. Probably some glitch or I just saw incorrectly where the state of the previous CI run updated only after the next started. 😞 Sorry about the confusion.

kamilkrzyskow commented 1 year ago

Hi @ultrabug, if you have the time please check out this alternate version: https://github.com/kamilkrzyskow/i18n/commits/refactor/config-hry-separate?since=2023-04-23 https://github.com/kamilkrzyskow/i18n/tree/gh-pages https://kamilkrzyskow.github.io/i18n/

It's based off the last PR I did, it introduces config recreation, separate search indexes, overrides JavaScript to load those separate search indexes. It seems to work for the most part, as I don't see any issues while navigating.

Issues I've seen:

Issues this setup solves (or at least tries to):

Let me know what you think 🙋. This solution uses some deeper internals of MkDocs, which are more likely to change over time, so it's rather "unsafe", could easily break (since MkDocs development started progressing again), but works splendidly, I think. EDIT3: Works splendidly for material and mkdocs or any theme with "on-page" search, readthedocs or any theme with separate search page breaks.

I also tried out the mkdocs theme to test out the JavaScript override, and I noticed a lack of language switcher? Are the links supposed to be inside the page content?

EDIT: So it turned out the default themes broke after not adding other languages to the search index in the default en language. Need to investigate it further. EDIT2: Fixed that, but I see that template files do not get processed by the on_post_page event, and don't get the path to the i18_link 🤔 EDIT3: Templates also fixed. But I tested it with readthedocs theme, which uses a separate search.html, which breaks with the separate search_index, because the search.html always points to a single language.

To fix this issue, the plugin would have to place the search.html in the language directory AND the searchbox form action would have to point to the correct search.html. I'm not sure if that is possible in a clean way 🤔 The plugin could edit the theme template file in a non intrusive way (not make the template rely completely on the plugin), but the again that's not clean at all 😮‍💨

EDIT4: OK, there is still hope, and maybe even an option for a cleaner solution, both on_page_context and on_template_context can influence the base_url:

base_url /mkdocs-static-i18n/
base_url .
base_url . 
base_url ..
base_url ../..
base_url ..

this could allow to create a i18n aware base, which would decrease the amount of JavaScript shenanigans

ultrabug commented 1 year ago

UPDATE:

In my other PR I introduced an example with a more generic setup, which run both folder and suffix based on the awesome-pages plugin tests. I still think that is a better methodology.

I took some time to think about a new testing strategy because I was not satisfied with the way tests were implemented so far.

I ended up with I hope a simple strategy where I'm comparing the nav, files, env and file representations and objects of reproducible mkdocs structures (files) that I pushed as test files.

This strategy uses vanilla mkdocs build logic without the plugin to produce a config, files, env and nav independently for every language version. Then using the same build logic (using pytest fixtures), I use the plugin to build the whole structure again. Then I compare the initial vanilla mkdocs build output files/env/nav to their i18n plugin counterparts.

This allowed me to catch a few bugs, fix index support while being sure that the output is strictly the same as it would if we were building each language independently.

This has to be extended still, but the test_control.py is here to demonstrate and for comments.

I hope it's clear and easy enough.

Thanks for your patience again

ultrabug commented 1 year ago

@unverbuggt I added support for the folder structure, can you please give it a try?

unverbuggt commented 1 year ago

@unverbuggt I added support for the folder structure, can you please give it a try?

I'm on python3.8, and it is missing "PurePath.is_relative_to" (available since python3.9 according to here. But I'll upgrade and give it a try.

Looks like my theme also needs an upgrade. I get " jinja2.exceptions.UndefinedError: 'list object' has no attribute 'items' " on "{%- for lang, display in config.plugins.i18n.config.languages.items() %}"

ultrabug commented 1 year ago

I'm on python3.8, and it is missing "PurePath.is_relative_to" (available since python3.9 according to https://github.com/shedskin/shedskin/issues/414. But I'll upgrade and give it a try.

Thanks indeed, I'll look for py3.8 backward compatible code

Looks like my theme also needs an upgrade. I get " jinja2.exceptions.UndefinedError: 'list object' has no attribute 'items' " on "{%- for lang, display in config.plugins.i18n.config.languages.items() %}"

I stupidly forgot to bump mkdocs requirements on setup.py that's why config validation did not warn you before... This is fixed.

Meanwhile I will write a mkdocs.yml config migration script to help users adapt their configuration to the new format.

ultrabug commented 1 year ago

@unverbuggt I just pushed a config_update_to_v1.py to help migrate your mkdocs.yml configuration to the new format.

python config_update_to_v1.py /path/to/mkdocs.yml

Hope this helps

ultrabug commented 1 year ago

I'm on python3.8, and it is missing "PurePath.is_relative_to" (available since python3.9 according to https://github.com/shedskin/shedskin/issues/414.

I addressed that issue, it should work on py3.8 now