-
Notifications
You must be signed in to change notification settings - Fork 14
ENH: Tags in body #84
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
Conversation
808ba76
to
310c954
Compare
pytest.raises() is very handy for testing exceptions.
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 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.
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. |
Oh, I see what you mean. It is possible to call 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 |
Thanks, is exactly the info I needed! |
3220daf
to
e43ee2e
Compare
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 😓 Also, not sure where I configure create tags on tests 😓 |
e54e826
to
268f183
Compare
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 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
|
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? |
268f183
to
b3a0c38
Compare
So I'm gonna pull this experiment out into it's own PR once I can get tests passing (maybe) |
So uh how important is it to fully preserve the exact amount of white space in the tags? 🙃 |
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 |
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 |
b3a0c38
to
2c2b829
Compare
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. |
2c2b829
to
778e744
Compare
dd4b342
to
c45873f
Compare
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 🤦♀️ ) |
dbae6c2
to
91a6e75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I think the only thing left is to update the changelog. It also might be worth a minor version bump (0.4.0) given the number of changes.
Edit: nevermind, I see there are already some milestones planned out for 0.3.1 and 0.4.0. I'll leave that up to Melissa.
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? |
I'm pretty sure the two tag PRs need to be reverted cause they're breaking the build 😱 |
Let's try and fix that before the release then. I should have some time to actually debug stuff this week, finally 😄 |
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