Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
merged 1 commit into from
Jan 2, 2024
Merged

ENH: Tags in body #84

merged 1 commit into from
Jan 2, 2024

Conversation

story645
Copy link
Collaborator

@story645 story645 commented Dec 4, 2023

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

@story645 story645 added the enhancement New feature or request label Dec 4, 2023
@story645 story645 force-pushed the tags-in-body branch 6 times, most recently from 808ba76 to 310c954 Compare December 4, 2023 07:08
test/conftest.py Outdated Show resolved Hide resolved
@JWCook
Copy link
Collaborator

JWCook commented Dec 10, 2023

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

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

JWCook commented Dec 11, 2023

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

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

Thanks, is exactly the info I needed!

@story645
Copy link
Collaborator Author

story645 commented Dec 11, 2023

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 😓

@story645 story645 force-pushed the tags-in-body branch 3 times, most recently from e54e826 to 268f183 Compare December 11, 2023 16:27
@melissawm
Copy link
Owner

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

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

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

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

@story645 story645 mentioned this pull request Dec 13, 2023
@melissawm
Copy link
Owner

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

story645 commented Dec 13, 2023

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

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 story645 force-pushed the tags-in-body branch 2 times, most recently from dd4b342 to c45873f Compare January 1, 2024 03:19
@story645
Copy link
Collaborator Author

story645 commented Jan 1, 2024

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 🤦‍♀️ )

@story645 story645 force-pushed the tags-in-body branch 3 times, most recently from dbae6c2 to 91a6e75 Compare January 2, 2024 19:03
@story645 story645 marked this pull request as ready for review January 2, 2024 19:06
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.

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.

@JWCook JWCook merged commit bf40fab into main Jan 2, 2024
@JWCook JWCook deleted the tags-in-body branch January 2, 2024 20:33
@melissawm
Copy link
Owner

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

story645 commented Jan 8, 2024

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tag list defined in multiple lines
3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.