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 in body #84

Closed story645 closed 8 months ago

story645 commented 9 months ago

closes #83 by adding support for tags in body of directive


.. tags::

   tag1, tag2,
   tag3, 

trying to get the tests to pass but opening so it's out there.

Also I know requiring the sep at the end of each line is kinda brittle but also probably the easiest. And I have no idea what I'm doing/tips on making this cleaner and testing the exception much appreciated.

Also I'm out of ideas on how to get the tests to pass - the tagged examples seem to build, but not the tagging index

JWCook commented 9 months ago

tips on making this cleaner and testing the exception much appreciated.

pytest.raises() is very handy for testing exceptions.

Also I'm out of ideas on how to get the tests to pass - the tagged examples seem to build, but not the tagging index

If adding onto the existing tests is giving you troubles, you might find it easier to add a new one (new input dir, output dir, and test function(s)).

story645 commented 9 months ago

pytest.raises() is very handy for testing exceptions.

I probably should have given more context here. This is an exception thrown by a directive, which as far as I know isn't something I can call programmatically. I know I can test it during the build process by adding a page where I trigger the error, but since you all test multiple input types I don't know if I should write a page of each type that triggers the error or if just triggering it for rst is good enough.

If adding onto the existing tests is giving you troubles, you might find it easier to add a new one (new input dir, output dir, and test function(s)).

I don't think that would help here b/c `test_tagged_pages passes and the index updating doesn't. I tested this by adding the tags to the docs and getting the same error.

JWCook commented 8 months ago

Oh, I see what you mean. It is possible to call TagLinks programmatically by mocking the initial directive state. This appears to work:

from sphinx.errors import ExtensionError
from sphinx_tags import TagLinks
from unittest.mock import MagicMock

def test_empty_taglinks():
    tag_links = TagLinks(
        "tags",
        "",
        {},
        [],
        0,
        0,
        "",
        MagicMock(),
        MagicMock(),
    )

    with pytest.raises(ExtensionError):
        tag_links.run()

It's probably also possible to test that by using the regular SphinxTestApp.build() instead, but I'm not sure how.

story645 commented 8 months ago

It is possible to call TagLinks programmatically by mocking the initial directive state.

Thanks, is exactly the info I needed!

story645 commented 8 months ago

I think I'm not following something -> if you're parsing the tag list in TagLink, what's happening in Entry/how come the Tag object is instantiated there, and does that mean the list gets parsed twice? ETA: I think so/this is why the index isn't building 😓 Ok, so trying to see if I can build the entry list from the parsed tags in TagLink, not sure if there's a way I can do that w/o holding a global {page, tags} dictionary.

Also, not sure where I configure create tags on tests 😓

melissawm commented 8 months ago

Actually holding a global page/tag dictionary may solve other problems I have ,so I would be ok with that. Is there a way to add that to the sphinx env object? Sphinx has very sparse documentation on these internal apis so I'm not really sure.

About the current architecture, there's probably better ways. I hacked something together from an existing idea so didn't really explore the concept. But basically, we

  1. Walk through all the documents (each document is an Entry)
  2. From each Entry, when the .. tags:: directive is detected, we add this Tag to the existing tags list (if it's not already there)
  3. TagLinks basically just replaces the .. tags:: directive with the contents of each tag
  4. The Tag class is responsible for writing the files with the TocTree entries of pages assigned to this tag.
story645 commented 8 months ago

sphinx env object

Hmm, going by the docs that's a https://www.sphinx-doc.org/en/master/extdev/envapi.html#sphinx.environment.BuildEnvironment object, so maybe as custom metadata?

story645 commented 8 months ago

Hmm, going by the docs that's a https://www.sphinx-doc.org/en/master/extdev/envapi.html#sphinx.environment.BuildEnvironment object, so maybe as custom metadata?

So I'm gonna pull this experiment out into it's own PR once I can get tests passing (maybe)

story645 commented 8 months ago

So uh how important is it to fully preserve the exact amount of white space in the tags? 🙃

melissawm commented 8 months ago

I don't think it's important - do you mean between tags or in a same tag? I think it's reasonable to assume spaces will be normalized

story645 commented 8 months ago

do you mean between tags or in a same tag?

Within tag -> 'tag\w\w\w 4' getting rendered as 'tag 4' - markdown apparently also normalizes the space automagically

eta: implemented this as second commit on #89

story645 commented 8 months ago

Just rebased this on top of #89 so that the global ness can be reviewed there and then this can be more cleanly rebased after.

story645 commented 8 months ago

So figured out that I could preserve the white space by stealing the sphinx-gallery trick of treating the tag list as one argument - so here I normalized it but really is basically same level of complexity whichever you want (but forgot that that's only true for rst anyway 🤦‍♀️ )

melissawm commented 8 months ago

Milestones are 100% flexible, I'm happy with a new release! Should I cut 0.4.0 soon or is there anything else you folks would like to include?

story645 commented 8 months ago

Should I cut 0.4.0 soon or is there anything else you folks would like to include?

I'm pretty sure the two tag PRs need to be reverted cause they're breaking the build 😱

melissawm commented 8 months ago

Let's try and fix that before the release then. I should have some time to actually debug stuff this week, finally 😄