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

ENH: tags stashed on env #89

Merged
merged 1 commit into from
Jan 2, 2024
Merged

ENH: tags stashed on env #89

merged 1 commit into from
Jan 2, 2024

Conversation

story645
Copy link
Collaborator

@story645 story645 commented Dec 13, 2023

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

  • moves the tag list for each doc to the app.env.metadata[docname]['tags'] dictionary, which makes the list global
  • patches Path with copytree so that pathlib can be used in conftest

Questions:

  • parsing the file in the directive because myst parser returns a blank block_text, my preference would be to just use the argument, but then it won't preserve the full white space -> is it very important that tag 4 get rendered as tag 4 and not tag 4 -> this is now the 2nd commit for comparison
  • not sure what the "WARNING: 'any' reference target not found" error means, given the badges still seem to kinda work?

@story645 story645 marked this pull request as ready for review December 13, 2023 07:32
@story645 story645 mentioned this pull request Dec 13, 2023
@JWCook
Copy link
Collaborator

JWCook commented Dec 13, 2023

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
Copy link
Collaborator Author

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

Only in the test_badges

story645 added a commit that referenced this pull request Dec 22, 2023
story645 added a commit that referenced this pull request Dec 22, 2023
@melissawm
Copy link
Owner

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
Copy link
Collaborator Author

story645 commented Dec 29, 2023

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

story645 commented Jan 1, 2024

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.

and using arguments, which normalizes spaces
@story645
Copy link
Collaborator Author

story645 commented Jan 1, 2024

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.

Copy link
Collaborator

@JWCook JWCook left a comment

Choose a reason for hiding this comment

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

These look like some nice improvements. Thanks for putting this together and fighting through all those testing issues!


for path in doc_paths:
entry = Entry(Path(app.srcdir) / path)
for docname in app.env.found_docs:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I had no idea this was a thing! Good find. I was looking for something like that for #58 but somehow missed it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks! found it when trying to figure out the environment variable https://www.sphinx-doc.org/en/master/extdev/envapi.html

src/sphinx_tags/__init__.py Show resolved Hide resolved
@JWCook JWCook merged commit f88c59c into main Jan 2, 2024
3 checks passed
@JWCook
Copy link
Collaborator

JWCook commented Jan 2, 2024

@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
Copy link
Collaborator Author

story645 commented Jan 2, 2024

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
Copy link
Collaborator Author

story645 commented Jan 2, 2024

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
Copy link
Collaborator Author

story645 commented Jan 3, 2024

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
Copy link
Collaborator

JWCook commented Jan 3, 2024

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
Copy link
Collaborator Author

story645 commented Jan 3, 2024

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
Copy link
Collaborator Author

story645 commented Jan 3, 2024

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
Copy link
Collaborator

JWCook commented Jan 3, 2024

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?

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

@story645
Copy link
Collaborator Author

story645 commented Jan 3, 2024

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
Copy link
Collaborator Author

story645 commented Jan 8, 2024

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

recipie->page
ingredients -> individual tags

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants