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

Multiple regressions in 1.0.2 #251

Closed
kamilkrzyskow opened this issue Sep 2, 2023 · 6 comments
Closed

Multiple regressions in 1.0.2 #251

kamilkrzyskow opened this issue Sep 2, 2023 · 6 comments

Comments

@kamilkrzyskow
Copy link
Contributor

The regressions might have been present in version 1.0.0, but I haven't verified it. I simply used the latest available version. Some of these issues could be intentional (though I hope not). Please consider this a mega-thread of challenges I faced while migrating to the newer version. 😃

Environment:

i18n 1.0.2 plugin config
  - i18n:
      docs_structure: suffix
      languages:
        - name: en - English
          locale: en
          default: true
          build: true
          nav_translations:
            Afsp: AFSP
            Anims: Animations
            Contribute: How To Contribute
            Daedalus tools: Daedalus
            General info: General information
            Genome: Genome (G3/R1)
            Lego: LeGo
            Vdfs tools: VDFS
            Zengin: ZenGin (G1/G2)
            Zgamepad: zGamePad
            Zparserextender: zParserExtender
        - name: pl - Polski
          locale: pl
          default: false
          build: !ENV [GMC_ENABLE_ON_PUBLISH, True]
          site_description: 'Gothic Modding Community to zbiór pomocnych materiałów do modowania gier Gothic i Risen.'
          nav_translations:
            Anims: Animacje
            Applications: Zastosowania
            Classes: Klasy
            Contribute: Jak Się Udzielić
            Examples: Przykłady
            Extenders: Extendery
            Extensions: Rozszerzenia
            Functions: Funkcje
            General info: Informacje ogólne
            Home: Strona Główna
            Plugins: Wtyczki
            Scripts: Skrypty
            Sound: Dźwięk
            Standalone: Samodzielne
            Syntax extensions: Rozszerzenia składni
            Tools: Narzędzia
            Tutorials: Poradniki
            Various: Różne
            Worlds: Światy
        - name: cs - Čeština
          locale: cs
          default: false
          build: !ENV [GMC_ENABLE_ON_PUBLISH, True]
          site_description: 'Dokumentace, návody a články zaměřené na modování her Gothic a Risen.'
File structure

https://tree.nathanfriend.io/

.
├── docs/
│   ├── some_dir/
│   │   └── some_more_dirs
│   ├── index.md # Default language AND fallback
│   ├── index.pl.md # Polish language
│   └── index.cs.md # Czech language
├── overrides/
│   ├── assets/
│   │   ├── images/
│   │   ├── javascript/
│   │   └── stylesheets/
│   └── main.html
└── mkdocs.yml

Issues:

  1. Relative Path Warnings:
    When building subsequent languages, warnings appear for relative paths to the overrides directory (no issues for the default language).

    INFO    -  mkdocs_static_i18n: Translated 23 navigation elements to 'pl'
    WARNING -  Doc file 'zengin/scripts/classes/c_info.md' contains a relative link
           '../../../assets/images/c_info_description.png', but the target
           'assets/images/c_info_description.png' is not found among
           documentation files.
    

    There's only a default language version of the file, and when the Polish version falls back to it, the path doesn't match. This warning wasn't present in plugin version 0.5.6* with MkDocs 1.5.2, although the error was partially there. With my MkDocs configuration using directory paths, the path gets altered with an additional ../ for the default language. In version 0.5.6, the paths in other languages were consistent with the default language. However, in version 1.0.2, they remain as is in the Markdown.

  2. Global Parameters:
    The previous default_language and default_language_only parameters were handy for CI and making swift local development changes using environment variables (GMC_DEFAULT_LANG, GMC_ONLY_DEFAULT_LANG). The new config seems to have reversed the behavior, making the default language obligatory. I might be able to work with that, but the subsequent issue is more concerning.

  3. Language Toggling Issues:
    Given that each language now has an individual toggle for default, it's not feasible to set a single environment variable. In addition, swapping the values temporarily (e.g., en <-> pl) is currently not possible due to the existing file structure:

    Exception: Conflicting files for the default language 'pl': choose either 'index.pl.md' or 'index.md' but not both
    

    The latest version seems to connect default with fallback. This results in a conflict when I aim to use index.pl.md as the default, while intending index.md to act as a fallback for the English version. I certainly don't intend to alter the file structure to accommodate files like index.en.md.

  4. Build Time Logs:
    The output displays individual build times for each build rather than a single cumulative log at the end.

    INFO    -  Documentation built in 8.28 seconds
    INFO    -  Documentation built in 35.00 seconds
    

    I recall somewhat resolving this to show only the final time log, but maybe MkDocs 1.5 altered the way duplicate filters operate.

These are my primary concerns for now. I'll update this list as I progress further.


*Note: While I did use a modified variant of plugin version 0.5.6, to the best of my recollection, none of the regressions mentioned were connected to those modifications. See modifications here.

@kamilkrzyskow
Copy link
Contributor Author

kamilkrzyskow commented Sep 4, 2023

Debugging the 1st issue I've found this:

# theme (and overrides) files
elif self.is_default_language_build:
i18n_files.append(file)

The assets from the overrides are only added to the initial build. This triggered a memory of the same thing I fixed in the patch to the refactor, so I dig further:

# theme (and overrides) files
elif self.is_default_language_build or file.src_uri.startswith("assets/"):
i18n_files.append(file)

You did merge my fix, but later decided to remove it in c2215b2 or perhaps it's a mistake missed because of the amount of changes.

With the fix restored, no warnings appear related to those files, and they are correctly resolved in the webpage. I don't understand the rationale of limiting the assets to only the first run. Perhaps performance to not copy the files multiple times? The issue is that MkDocs relies on the files.src_uris to look for the valid assets when it resolves links in Markdown and you're reconstructing the mapping for each language build.


I do realize this issue could be also related to other directories, assuming someone uses files/ as their assets/ prefix. So perhaps adding some global list of strings (mkdocs config setting) with prefixes to always include would be beneficiary.

@kamilkrzyskow
Copy link
Contributor Author

kamilkrzyskow commented Sep 4, 2023

Investigating the 3rd issue, I've confirmed for myself that the default language is closely related to the fallback, especially the default file without any suffix locale is always assumed to be using the default locale. I've tried some things to "revert" the behaviour to the previous way, but I either introduced a bug somewhere else (like for example the alternate switcher also now relies on the extension locale, I think) or the performance got worse.

So I've accepted that there needs to be some sort of compromise here between the new and old behaviour.

Locally, I've decided to add a global development_locale config option, which allows to forcefully override any other default and build options, so that the development locale would be the default and only language being built, additionally the development build avoids the conflicting files scenario by prioritizing the filename.locale.md files instead of the filename.md files.

Therefore this global option partially fixes the 2nd and 3rd issues, I've written above.
It allows to quickly change the locale to a different one using environmental variables, and it allows to keep the older file structure setup while testing other languages.
I don't need the option to build all of the languages using another language as the default one, because English (without extension) will always be used as the default one for the deployment.

This was referenced Sep 4, 2023
@kamilkrzyskow
Copy link
Contributor Author

kamilkrzyskow commented Sep 12, 2023

The 1st issue, has been dealt with in #256
The 4th issue, has been dealt with in #257

@ultrabug
Copy link
Owner

@kamilkrzyskow since quite a lot have been done so far, would you be okay to close this PR and open a separate new one for the hopefully last problem you'd like us to solve?

@kamilkrzyskow
Copy link
Contributor Author

kamilkrzyskow commented Sep 18, 2023

Sure, I'll edit the OP and move the last issue to a separate issue.
EDIT: Before that I need to rebase to the current branch, maybe the file suffix handling fix somehow helped with the last issue 🤔

@kamilkrzyskow
Copy link
Contributor Author

Suppressed with #262

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 a pull request may close this issue.

2 participants