Order by title, global fallback for attributes

[kamilkrzyskow]

Possibly fixes #56

Hello 👋, our team wanted to run this plugin with the https://github.com/ultrabug/mkdocs-static-i18n plugin, and I wanted to run this setup specifically:

docs/
    - .pages
    - index.md
    - index.lang.md
    - section_dir1
        - .pages
        ... more_dirs
    - section_dir2
        - .pages
        ... more_dirs
    - section_dir3
        - .pages
        ... more_dirs
    - section_dir4
        - .pages
        ... more_dirs

The issue I've encountered was that to get order: asc working in every section_dir and more_dirs I would have to add .pages to every directory with that setting. So I've added a global option, which allows to omit setting this attribute separately. Issues: My solution allows to set a different order in the .pages file, however only to the other valid setting asc/desc. Turning it off to get the default ordering in a given directory while the global fallback is active is impossible.

Same thing applies to the other setting sort_type

Additionally, as the multilingual context of our docs, we wanted to be able to sort by title, as the file names are in English, but the title in another language could have a different order in the nav. So I've added the option to order by title. To achieve that the solution loads the titles the same way as MkDocs. I've injected the solution into the meta gathering process to avoid more items iteration. The setting can be turned off / on for any directory and globally. The title_ordering option name might not be the best, but I wanted a toggle instead of a choice setting like it is for order and sort_type. I had order_by -> ["filename", "title"] in mind, but I'm not sure what other options are there to set that are reasonable. I can make adjustments of course if you'd prefer to have the choice setting to align with the rest 😉. Issues: The setting doesn't work without order/sort_type enabled, so maybe it's weird that 1 option isn't sufficient to enable it.

I added some tests to check that the new settings do influence the behavior of the app, I hope they're enough.

---

I started with running the poetry install, and it was probably the worst part of this process. First time encountering it and straight up I got an error when installing the project on Python 3.11 locally on Windows. I updated some dependencies for newer Python versions, and it worked, however it was not fun. It seems to be a brand-new issue https://github.com/python-poetry/poetry/issues/7583 Later a test case was failing, turned out it was invalid (or maybe invalidated), so I turned it off for newer MkDocs versions. Later it turned out that poetry itself stopped supporting Python 3.6, so I disabled testing for it in the workflow. Later Windows runners didn't work with a poetry is an unrecognized command error, I tried to fix it using the guide from the provided GitHub Action, but that also didn't work (got a .venv directory doesn't exist error)... So I've just redone the workflow file "my way" after a few hours (or was it days 😮‍💨) of trying to get other things to work. I removed the GH Action and added caching. However, I'm not entirely sure if it will increase performance in this project, because the workflow file is run rather seldom. Backlog.

---

As it is already not supported by poetry or ubuntu-latest, you might want to completely remove the 3.6 support from the dependencies. This would allow to use pytest-xdist and slightly speed up running tests concurrently.

[kamilkrzyskow]

The title_ordering setting lacks some warnings about not enabled order/sort_type or it could enforce order: asc if disabled. I'll change it to a draft and wait for more feedback then.

[lukasgeiter]

Hi there, and thank you for this PR!

I'm sorry to hear that you had this many issues to get everything to run. Thanks for powering through it though! I think I'm going to take a stab at the workflow as well. I'm hoping to come up with something that is a bit more straightforward.

---

Issues: The setting doesn't work without order/sort_type enabled, so maybe it's weird that 1 option isn't sufficient to enable it.

Is there a reason why it couldn't work as a standalone setting?

[kamilkrzyskow]

Good luck with the DevOps shenanigans ✌️.

After thinking about it, cache doesn't make a lot of sense if the workflow is run once per 3+ months, since the dependencies will surely change and the cache will have to rebuild adding an overhead. Also GitHub could delete rarely used caches too 🤔. So maybe consider adding a schedule to the workflow, to run it at least once per week or two. This would allow to spot issues, like the one I had more quickly. Otherwise just delete the caching 👌

Is there a reason why it couldn't work as a standalone setting?

https://github.com/lukasgeiter/mkdocs-awesome-pages-plugin/blob/a54ae48fce696cd55ca6c3e21632642d0a2bd097/mkdocs_awesome_pages_plugin/navigation.py#L93-L98

Currently the toggle switches the key function, and then the items.sort function uses that key function to reorder the items based on the order/sort_type settings. Like I said in my last message, I could just enforce order: asc if it's not set.

https://github.com/lukasgeiter/mkdocs-awesome-pages-plugin/blob/a54ae48fce696cd55ca6c3e21632642d0a2bd097/mkdocs_awesome_pages_plugin/navigation.py#L102

Oh, I just noticed it when writing this. If the order is None it would just automatically order it by asc, because of the reverse condition 🤦. Then, sure, it can work as standalone ✔️.

[kamilkrzyskow]

Moving the code around to make the setting standalone like the other 2 settings, revealed a bug. 1 test case failed, where there is no global title_ordering and local Meta title_ordering is set to True. In this case the titles aren't gathered during the meta gathering, and it crashes during the ordering process since the local Meta tries to order titles that aren't loaded.

https://github.com/lukasgeiter/mkdocs-awesome-pages-plugin/blob/a54ae48fce696cd55ca6c3e21632642d0a2bd097/mkdocs_awesome_pages_plugin/navigation.py#L227

Currently investigating if I can make it work without the title_ordering having to be enabled globally to then be turned off locally, and just allow to enable it per Meta instance.

[kamilkrzyskow]

Made the title_ordering setting standalone and dealt with the bug. I'm liking the name less and less 😞maybe a bool order_by_title or str order_by would be better. Also adding an "default" option for the order/sort_type could be a good solution to allow users to override the global settings 🤔.

Once you approve of the changes I'll squash the fixup commits and force push to the branch :v: #ForcePushEnjoyer

EDIT: I opted into a more succinct name title_order.

EDIT2: After pondering over it, I think there might be some edge-case need for ordering the files by file creation or revision date / file size 🤔. Therefore my opinion on having this as a boolean toggle weakened. but I will wait a bit with the final decision.

[kamilkrzyskow]

OK, I think I'm finished for now. Summary of changes since initial PR:

• The boolean title_ordering setting is now a str order_by setting, with title and filename being possible choices. The change was made to make it more future-proof,
• The setting is now standalone just like the other 2 order and sort_type,
• I've added 2-3 tests,
• Fixed code style of some additions.

There is still the issue mentioned before:

My solution allows to set a different order in the .pages file, however only to the other valid setting asc/desc. Turning it off to get the default ordering in a given directory while the global fallback is active is impossible.

I don't think the possibility to do that is very useful, therefore I didn't add a default choice for either order or sort_type. But I can add it later if you want, because the usefulness isn't exactly 0.

I'll squash the fixups and open up for the review process. A backup of the separate fixups is in a separate branch.

[lukasgeiter]

Alright, I've solved the various dependency and workflow issues on the master branch - please rebase 🙂 I opted to not use caching for simplicity sake. I also noticed that installing the dependencies usually takes 10 seconds or less which is definitely fast enough.

---

Thanks for the work you've been doing in the meantime! Hopefully I'll find the time soon to actually review them.

[kamilkrzyskow]

So my misery of dealing with DevOps won't even be remembered by the git history 👀💦? Kind of sad, but anyways I've looked at your changes and the run action at the end. I see that the guide I've linked previously is partially correct, but breaks at the "with cache" part 😆, so it would've worked if I didn't try adding it.

I've noticed also that it's not exactly 10s or less, the Install Poetry step, which uses the GitHub Action script often takes 30s or more. My shortest action runs took 2 minutes (with cache loading), the longest took almost 8 minutes (with cache creation). This might showcase how good the cache is, or how bad the speed variability of the GitHub runners + in your case of the Install Poetry script is 😅.

I see that you've updated the lock file to the new poetry version, updated the python version to >=3.7, but didn't update the dependencies so MkDocs is still at 1.2.3. Nonetheless, I'll keep the test case deprecation, because there might be another person that installs newer dependencies before doing changes to the project.

EDIT: Since it's >=3.7 consider adding pytest-xdist and then run it with 2, 4 or 6 (💦 maybe auto will be better) workers, however it would save up to 5 seconds in most cases, so maybe it's a too small improvement. I've updated all of the commit messages to conform better with the current history, so please don't squash them into 1 PR commit.

EDIT2: Not worth it creating a new issue, so I'll write it here. I tried doing a clean install with the updated files and still get an error.txt. The error indicates some kind of "Permission Denied", but that's not the case since I even tried running it with Administrator rights 👀 and still same issue. There is the ~. in the charset for some reason that creates some kind of error in the path lookup or maybe the file is somehow used/protected by poetry itself. Based on this older workflow run this seems to be a problem limited to Windows or maybe my approach, since the GitHub Action script installs poetry in a way that doesn't install the newer charset-normalizer module 😮‍💨.

I'm installing poetry via the manual method, which is the most straight-froward to me. In git bash:

python -m venv venv
source venv/Scripts/activate
pip install poetry
poetry install
[Error]

To solve this a poetry update is required that updates the dependencies, and therefore there is no uninstalling of the old / different module that poetry also uses. However, I'm stil unsure if that's a problem with my installation method, some upstream issue with poetry blocking something or even some edge-case issue with that specific module.

I've spent already too much time initially with the DevOps shenanigans, so I'm for the quick fix approach, which is running poetry update. I'm not really planning to investigate any further.

[kamilkrzyskow]

Yet another update of my vision, didn't consider it before since the additional setting was of a different boolean type.

Having order, sort_type and order_by as a str type, I think that 2 out of the three could be combined to share a naming pool. Combining order and order_by into a single order choice with "asc", "desc", "asc_title", "desc_title" would be still reasonable 🤔 For the reverse check reverse="desc" in order could be used.

This is of course a strong matter of preference, so I'm not sure which one would be more future-proof.

[kamilkrzyskow]

1 week has passed, ping for exposure @lukasgeiter 🙋

[kamilkrzyskow]

I tackled the second issue first, and refactored the meta gathering to have cleaner code. After solving the first issue, via dynamically retrieving the title during order, it turned out that no refactor to the meta gathering is needed 😅. I don't like this approach of looping over the NavItems multiple times, but I guess you prefer cleaner code in this case, because the performance difference will likely be negligible.

🙋 Let me know if I should move the get_title to the utils module. Also if I should omit any meta gathering refactor, but I think that this version is cleaner ✌️

[kamilkrzyskow]

@lukasgeiter 🙋 Hi, just a reminder ping. I'm still waiting for feedback to my feedback ✌️

[kamilkrzyskow]

@lukasgeiter 🙋 H, just a reminder ping, I'm still waiting for feedback to my feedback ✌️

[lukasgeiter]

Thanks for your work and sorry for leaving you waiting. The _gather_metadata refactor looks good to me 👍 I'm still not too excited about the copied code for titles, but I don't have a better suggestion so I'm willing to merge it in its current state. 🙂

[kamilkrzyskow]

Happy Easter ✌️ Thanks for the positive review 🤘 I've cleaned up the commits, moved the meta gathering refactor to its own commit, therefore the add order_by commit isn't polluted. I think that all commits have valid names and can all be merged 👍

[lukasgeiter]

Released with v2.9.0