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

Add use-page config option #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jannschu
Copy link

The option allows to use a different page for the section page instead
of the first child.

Copy link
Owner

@oprypin oprypin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. Overall it is good. But I have sooo many nitpicks.

In addition to the rest, this change should have tests along with it.
And even the existing tests are failing now.

To go along with this line

config = dict(nav=golden["input"], use_directory_urls=use_directory_urls)
you'd probably need

plugin.config = {"index_file": golden.get("index_file")}

You will need to run tests. For that you can either run poetry install; .tools/hooks/pre-commit, or install dependencies directly and run pytest however you want.

README.md Outdated Show resolved Hide resolved
mkdocs_section_index/plugin.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
mkdocs_section_index/plugin.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
mkdocs_section_index/plugin.py Outdated Show resolved Hide resolved
@jannschu
Copy link
Author

jannschu commented May 28, 2021

Just saw you comments after my last push, I'm on it :)

@jannschu
Copy link
Author

I addressed your comments, but I have the feeling that previous_page is now wrong if index_page does not pick the first child. I'll add a test case to be sure.

@jannschu jannschu force-pushed the page-option branch 2 times, most recently from 6fa6480 to bf8d0fd Compare May 28, 2021 19:39
@oprypin
Copy link
Owner

oprypin commented May 28, 2021

Oh wow that's a lot of logic now :(
Do you think anyone will actually need the ability to pick index pages from the middle?
The option could just require it to be index.md. Doesn't that completely cover your use case?

@jannschu
Copy link
Author

Oh wow that's a lot of logic now :(
Do you think anyone will actually need the ability to pick index pages from the middle?
The option could just require it to be index.md. Doesn't that completely cover your use case?

Actually index.md would be more than enough for me. So I'm fine with restricting it to that, but the logic is basically "fix a doubly linked list".

I'd even argue that if there is no index.md the first page should not become the section page by default since it then depends on alphabetical order but there might be use cases for that.

@oprypin
Copy link
Owner

oprypin commented May 28, 2021

I'd even argue that if there is no index.md the first page should not become the section page by default since it then depends on alphabetical order but there might be use cases for that.

Right-- I was thinking that too, but I forgot after seeing your code because it looked totally reasonable :)

Well, if you search for existing usages, people sometimes intentionally use non-index-named pages as the index, but the prerequisite is that their nav is specified manually.

I could make this the default only if the nav is not specified manually, but implementing that is dubious and also I think it's super rare that people just leave their nav alphabetically sorted.

So this should remain just an option.

And sorry but I think we should not keep the logic to reorder the pages. It should still be required to be first.

)
if nav_src == "derived_nav" and index_file != "default_index_file":
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding actual files to the real example and branching all of this logic, I think the test should be separate and new.

If you need to use files from the real example, you can first create a directory for it then copy those files and add more.

shutil.copytree(example_dir / "docs", str(tmpdir))

noindex_dir = (tmpdir / "docs").mkdir("z_noindex")
(noindex_dir / "a.md").write_text("# a")

then change docs_dir=tmp_dir,

Or if you don't need the real example, just don't copy the files

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

@jannschu
Copy link
Author

Just thougth about the case where a previous plugin might have reordered items such that index.md is no longer first. This could lead to unexpected results if we just check if the first child is index.md with that option. But we could omit a warning in such a case, but actually moving these element in a doubly linked list seems rather clean to me. But it's on you of course, just my final thoughts on this.

By the way if a plugin reorders children after this plugin results are most likely wrong. We could also omit a warning maybe if popular plugins which could do that are behind this plugin.

@oprypin
Copy link
Owner

oprypin commented May 28, 2021

The warning is there

log.warning(
"It seems that the effects of section-index plugin have been lost, because another MkDocs plugin re-wrote the nav! "
"Re-order `plugins` in mkdocs.yml so that 'section-index' appears closer to the end."
)

Though it doesn't detect subtle reorderings. But I think it's not unreasonable to expect the plugins to coexist anyway. Currently all mav reordering plugins I know rewrite the whole nav.

The option allows to use a different page for the section page instead
of the first child.
@solonovamax
Copy link

What's the status on this? Because it fixes #6, which is a very annoying issue for me.

@oprypin
Copy link
Owner

oprypin commented Sep 7, 2021

Hmm the status is that I'm a bad human. I didn't follow up on this.
The feature is needed, but I wasn't sure how exactly it should look like.

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 this pull request may close these issues.

3 participants