search

ultrabug / mkdocs-static-i18n

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

Refactor patch - navigation and related fixes #220

Closed kamilkrzyskow closed 1 year ago

kamilkrzyskow commented 1 year ago

@ultrabug I think it's ready for merge, unless you're still unsure about something. ✌️

kamilkrzyskow commented 1 year ago

Hi @ultrabug, I've cleaned up the PR from failed commits, finally fixed the homepage assignment and added a fix to properly show the final time log. I did a force push, but kept a backup of the old branch just in case. Here you can see a comparison between them

The homepage assignment was fixed with allowing to check for item.url == "" because from my testing I noticed that the main/default homepage path does always have an empty url inside the object. The issue I've been facing turned out to not be related to any plugin. Having the index homepage inside a sub Section caused MkDocs to ignore it in automatic assignment.

https://github.com/ultrabug/mkdocs-static-i18n/blob/29e169e062ac8635b4ff6497e0d3824de5cbd708/mkdocs_static_i18n/reconfigure.py#L264

As for the time logging, it showed the finished time from an inner build, and the log message was added to the DuplicateFilter, which blocked every other build time logging. I basically flipped it, I block the inner ones and allow the last one. :v:

https://github.com/ultrabug/mkdocs-static-i18n/blob/29e169e062ac8635b4ff6497e0d3824de5cbd708/mkdocs_static_i18n/plugin.py#L171

I think there won't be any more additions to this PR, if I have anything I shall keep it for another PR.

Also as a side note, I got tired of having very limited filtering options when it comes to logging in MkDocs in general. https://github.com/mkdocs/mkdocs/issues/2876#issuecomment-1518206769 I've made a hook, which acts as a manager to filter the log in different ways. If I needed something more than there is in the base example, I simply added more conditions inside the filter function. Maybe you'll get some use from it too :v:.

kamilkrzyskow commented 1 year ago

I spoke too soon 😞I still got the urge to clean up and extract a part of the navigation homepage code from the reconfigure function back into the on_nav event. I also considered adding an option to disable the unsupported lunr.js warning completely for a given language, but ultimately didn't go through with it, I'll get back to the idea another time. Wasting so much GitHub Action time...

ultrabug commented 1 year ago

(FYI I'm coming back to this ASAP once I'm done with other things that require my full attention too)

ultrabug commented 1 year ago

@kamilkrzyskow thanks for your patience and continued work. This PR is going too far IMHO since we did not settle and cover the tests of the initial WIP PR.

I have nothing against trying to change the way we build and trying to get those search indexes lean. But this should be done once we have a clear and functional initial WIP PR.

Else we'll end up iterating over and over while nothing gets properly tested.

I'm gonna comment my latest commit on my initial WIP PR since I'm happy about what I just pushed.

kamilkrzyskow commented 1 year ago

Hi @ultrabug, this PR doesn't influence the inner build process and doesn't make the search indexes leaner at all, I only mentioned that I'm working on it on a separate branch based off of this PR. So I'm not sure what you mean with that it's going too far, since this PR contains purely small fixes?

You also once again copied a piece of code I fixed, improved it even more, and added to your solution, which created a merge conflict with this branch, so I would have to remove my changes (an hour of debugging and fixes) to get it merged. Sorry, but I hate doing git like that, back-and-forth, please ping me when you're ready to merge this PR, I shall then rebase.

I'm talking about this: https://github.com/ultrabug/mkdocs-static-i18n/blob/f938671b9c5372c97f0ba6a0b957d3884bc4b398/mkdocs_static_i18n/suffix.py#L58-L60 https://github.com/ultrabug/mkdocs-static-i18n/blob/6e69a58425c689655975ed530cd2f874a5f58b8a/mkdocs_static_i18n/suffix.py#L63-L65

https://github.com/ultrabug/mkdocs-static-i18n/blob/f938671b9c5372c97f0ba6a0b957d3884bc4b398/mkdocs_static_i18n/suffix.py#L161-L162 https://github.com/ultrabug/mkdocs-static-i18n/blob/6e69a58425c689655975ed530cd2f874a5f58b8a/mkdocs_static_i18n/suffix.py#L143-L145

I thought I could help you release suffix support faster, but it seems that won't happen soon. Therefore, I went back to the pre-refactor plugin version, and I added support for site_description override + I'm currently making sure that my changes to support the Material social card plugin are working. I shall open up a PR for this tomorrow. Additionally, I've opened https://github.com/ultrabug/mkdocs-static-i18n/pull/205 for review, because maybe you forgot about it since it was a draft and you focused on the refactor WIP PR, which should IMHO be only a side hustle.

I'm still at the point, where commits in git history are still precious to me, proof of overcome hardship. I would also prefer to be marked as a co-author instead of thx to @kamilkrzyskow if my ideas were of value. You can get my email from the git history or use https://api.github.com/users/kamilkrzyskow to get the user ID and generate a GitHub noreply handle Co-authored-by: kamilkrzyskow <34622465+kamilkrzyskow@users.noreply.github.com>

I've heard of the https://www.conventionalcommits.org/ which you seem to follow, and if you're planning to squash the whole refactor WIP PR during merge, then I guess I should abstain from doing any commits. I only squash amends and "duplicate" commits, which are present in https://github.com/ultrabug/mkdocs-static-i18n/pull/205 and I wanted to squash them myself after your review of my draft, I kept them there to make following my thought process easier.

Like I said previously, I'm strict about commit attribution, and to assure you I'm not targeting you specifically, I did have a rather chaotic discussion about it here too, so please don't take it personally ✌️

ultrabug commented 1 year ago

I thought I could help you release suffix support faster, but it seems that won't happen soon.

I agree and I'm not satisfied with how we move forward together and I feel responsible for it. I won't go into arguing about this PR and code copy/pasting as it won't be helping to write yet another comment about that.

So let's try something else if you agree: are you on discord or somewhere where we could discuss faster than Github comments maybe?

If not, I'll answer here and propose something hopefully better so we can get this major rewrite done once and for all.

Thank you, we'll make it :)

kamilkrzyskow commented 1 year ago

I agree and I'm not satisfied with how we move forward together and I feel responsible for it.

No need to feel responsible, apart of rewriting the code written by me instead of merging it twice (a cherry-pick would be enough if the whole PR wasn't good), I have nothing against your workflow, I'm just that kind of person, which needs to be interested in something to do it, and I'm not interested in the whole refactor to fully cooperate with you, just unlucky circumstances. ✌️

Also yes, I'm a daily Discord user, but I'm not willing to invest my time into a fully tested and complete refactor, so I guess there is no need for faster communication. Initially I wanted to just comment about things that could be improved, but then as I said in my comment here my goal was to make the refactor functional (suffix structure specifically) to allow me to introduce Material's social cards into my team's documentation. This PR made it somewhat functional restoring nav links working, removing warnings etc., but it turned out that I missed something about how the "unlocalization" of files works, hence you introduced more changes after the PR. I didn't want to commit to a strict procedural testing process, so I jumped ship and went back to the pre-refactor version and introduced support for site_description override and did some "bigger" changes too to facilitate the support of the Social Cards plugin, the testing suite still somehow works 🤷

I shall open up a PR for this tomorrow.

Well, I did the changes, but didn't test them with the folder structure, and I'm not willing to write tests for it either (especially when the newer testing approach from the Sitemap PR isn't merged). Therefore, anticipating yet another PR rejection, I just abandoned the idea, or let's say I'm still procrastinating on it 😉

Later, if I have the time, I will comment on some parts of the code that I see issues with, so just like I did before, giving out unsolicited advice 😏

As a side-note, it turned out that the social cards plugin uses the page.file.src to generate the path to put the images and the meta head tag url to that image. This became a problem for the pages in another locale that weren't yet translated, hence they used the original default language source file. This re-usage of the source path leads to overwriting of the default language images by the other locale (which uses a different site_description). Maybe you'll come up with a way to handle such cases better than my decorator wrapper approach, but to me this rather unique case requires it.

https://github.com/kamilkrzyskow/i18n/commit/af9b1fc81fc7e615f1987e29e832963f2a067532

ultrabug commented 1 year ago

Ok @kamilkrzyskow thanks for clarifying ; I'm off this week, will move on next one hopefully

jonnyforestGIS commented 1 year ago

I @ultrabug a gentle ping about the status of this PR. Do you have plans or time to review this PR and merge?

Cheers

ultrabug commented 1 year ago

@jonnyforestGIS yes I have many things going on at the same time so I'm pushing this PR in the backlog

Another reason is that we are working on the MkDocs 1.5 release which will change some internals so I'd rather make this PR target the latest MkDocs version instead.

I should spend again some time on this PR by the end of the month!

jonnyforestGIS commented 1 year ago

Hi @ultrabug thank you for your quick feedback and thank you for your effort. Cheers

ultrabug commented 1 year ago

Let's move on, merging to @kamilkrzyskow gets credits ofc

I'll update the code on the other PR.