melissawm / sphinx-tags

A tiny Sphinx extension that implements blog-style tags for documentation.
https://sphinx-tags.readthedocs.io/en/latest/
MIT License
44 stars 11 forks source link

ENH: tags stashed on env #89

Closed story645 closed 8 months ago

story645 commented 8 months ago

Pulled out of #84 for easier review, this PR:

Questions:

JWCook commented 8 months ago

my preference would be to just use the argument, but then it won't preserve the full white space

I agree, I don't think that's very important since Markdown already collapses consecutive spaces.

not sure what the "WARNING: 'any' reference target not found" error mean

I know I've seen that before. It might be something along the lines of "something was interpreted as a :ref: that wasn't intended to be a :ref:." It could be either a minor syntax problem or something that could be ignored. Are you seeing this in the tests, or the doc build for sphinx-tags itself?

Either way, not a very helpful error message. I'd rate it 2/10.

story645 commented 8 months ago

Are you seeing this in the tests, or the doc build for sphinx-tags itself?

Only in the test_badges

melissawm commented 8 months ago

I'm not sure what's happening. Locally, I am seeing the "any" error on the regular docs build, and the tags are collected but the links are not actually generated. I'll try debugging some more if I find the time today or tomorrow but I'm going to be out all next week on vacation. If you folks find a fix please just merge and move forward with it, the general idea is good on my side 😄

story645 commented 8 months ago

but the links are not actually generated.

~Do you know on which page? I built the docs and the links seem to work for me but not sure where I'm supposed to be checking.~ Never mind, saw that it's the tags not getting written to tagindex.md And somehow setting tags_create_badges to false removes the error 😕

story645 commented 8 months ago

To make this even more confusing, the nodes are completely identical https://gist.github.com/story645/d6a4698ff8d743a937fc77716573a218/revisions?diff=split&w= on main and this branch

story645 commented 8 months ago

And the difference in build is that on a working build, myst sees the files as changed

building [text]: targets for 7 source files that are out of date updating environment: 0 added, 7 changed, 0 removed

while for badges it's not updating the environment:

updating environment: [new config] 3 added, 0 changed, 0 removed

Basically, the env dictionary is only populated after update_tags is run.

story645 commented 8 months ago

Ok, what fixed it was adding a second build in an isolated test (which I think test_general also doesn't work w/o the test_build test). Don't love this but am also so confused.

JWCook commented 8 months ago

@story645 Okay, I think I see what's going on. Those errors (File Not Found: _tags/*.html and WARNING: 'any' reference target not found) were legit, and tag pages aren't getting generated in normal builds. For example:

rm -rf docs/{_tags,_build}
make -C docs html
ls docs/_tags

At the point where we're checking app.env.metadata["tags"] in assign_entries(), I think the builder-inited event hasn't yet run, so TagLinks.run() hasn't been called yet, so that metadata is empty. It looks like this will only be populated if the build is run twice within a single SphinxApp instance, which is why test_tag_pages() works after test_build(), but not on its own.

One possible fix may be to connect update_tags() to a different event later in the build process (reference).

story645 commented 8 months ago

One possible fix may be to connect update_tags() to a different event later in the build process (reference).

I figured it'd be something like that, but I wasn't sure how to do it.

story645 commented 8 months ago

One possible fix may be to connect update_tags() to a different event later in the build process (reference).

Looks like I can't maybe get this working w/ env-updated so I can try to PR that

story645 commented 8 months ago

So been trying stuff and nothings been working so I think these two PRs will need to be reverted until I can figure out what went wrong (cause I remember it working at some point 😓 )

JWCook commented 8 months ago

I also looked into this a bit and haven't quite figured it out yet, but I do feel somewhat closer to understanding how the build events work. Honestly I wish Sphinx's docs on the topic were better!

We probably wouldn't need to revert all of your changes, but maybe we could add the parsing back to the Entry class if we can't figure it out by the end of the week?

story645 commented 8 months ago

but maybe we could add the parsing back to the Entry class if we can't figure it out by the end of the week?

Either that or a non env global variable maybe?

story645 commented 8 months ago

Another option is refactoring all the things into the kinds domain/index/directive model here https://www.sphinx-doc.org/en/master/development/tutorials/recipe.html b/c I think the objects are more or less aligned w/ that.

JWCook commented 8 months ago

Either that or a non env global variable maybe?

The order of operations is still the tricky part there: we need TagLinks to run first, then update_tags(), and all before toctree runs so it can pick up the pages generated by update_tags(). Using the source-read event (run for each individual document) with a higher priority than toctree might work... except for the tag overview page since it needs access to all tags.

Another option is refactoring all the things into the kinds domain/index/directive model here

That looks promising! So would that look something like this?

story645 commented 8 months ago

except for the tag overview page since it needs access to all tags.

Yeah, I tried using doctree-read and where it'd keep getting stuck was that index is looking for the tagindex page so I tried writing tagindex in parts (empty header than the rest) and it crashed but I'm realizing now I might be able to swing something with includes 🤔

That looks promising! So would that look something like this?

1 index class for tag pages
1 index class for the tag overview page
A domain class containing those indices and the tags directive

I think so? or something like the Index holding the tags<->pages look up. Forgot the formal name, but it's a bidirectional graph of the form:

tag1 tag2 .... tagn
page1 --- ---- ----
page2 --- ---- ----
.... --- ---- ----
pagen --- ---- ----

Where a 1 in the matrix means that that tag is on that page.

story645 commented 8 months ago

Scratch, you were correct about two indices, with a mapping like:

recipie->page ingredients -> individual tags