search

liran-funaro / sphinx-markdown-builder

A Sphinx extension to add markdown generation support.
https://pypi.org/project/sphinx-markdown-builder
MIT License
41 stars 19 forks source link

Support glossary directive #10

Closed adam-zlatniczki closed 10 months ago

adam-zlatniczki commented 1 year ago

I've included a support mechanism for glossaries. Unfortunately I didn't run the tests, I just couldn't manage to make the forked repo work as it should (poetry throws an exception, KeyError 'name' that relates to config["name"] - I have no idea why, there's a name attribute in the pyproject.toml; including a devcontainer might be a nice way to circumvent such issues for future contributors). I had to try the code by overwriting the files in an installed version of the package, where it gave me results that I think are correct. Please take a look at the code, and if you find it useful, try running the tests to make sure it doesn't break anything before a potential merge. If you have any ideas/suggestions on the pull request, feel free to share them.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 7539248394

Totals Coverage Status
Change from base Build 7179832233: 0.3%
Covered Lines: 721
Relevant Lines: 721
💛 - Coveralls
liran-funaro commented 11 months ago

Thank you for the PR. I'm sorry it took so much time to reply. I've been on vacation, followed by a war.

Can you please add test cases for the new handlers? See https://github.com/liran-funaro/sphinx-markdown-builder/blob/main/CONTRIBUTING.md#contributing-tests

adam-zlatniczki commented 11 months ago

Yes, definitely. I've seen that the changes didn't pass black either, it seems I forgot to turn formatting on in my IDE, I'll see to that as well.

Take care in these trying times.

On Sun, 3 Dec 2023, 14:47 Liran Funaro, @.***> wrote:

Thank you for the PR. I'm sorry it took so much time to reply. I've been on vacation, followed by a war.

Can you please add test cases for the new handlers? See https://github.com/liran-funaro/sphinx-markdown-builder/blob/main/CONTRIBUTING.md#contributing-tests

— Reply to this email directly, view it on GitHub https://github.com/liran-funaro/sphinx-markdown-builder/pull/10#issuecomment-1837485570, or unsubscribe https://github.com/notifications/unsubscribe-auth/AITBLK3FI4ZNWOSQDLAGPLLYHR7FPAVCNFSM6AAAAAA6VF7WROVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZXGQ4DKNJXGA . You are receiving this because you authored the thread.Message ID: @.***>

adam-zlatniczki commented 11 months ago

I've added some tests, formatted the content with black, and also made sure that pylint doesn't find any problems. Unfortunately, I had to disable a few pylint rules in a few code blocks, because they were intentionally made in a way that was conflicting with those rules. Don't worry about the number of commits, it just took me a few tries before the changes were able to pass the CI, the content didn't change much.

liran-funaro commented 11 months ago

Thank you for this PR. I think adding a glossary is useful. I greatly appreciate your effort and want to encourage you to contribute further. Eventually, I decided to go with a simpler approach (only minor changes, see this commit).

This achieves a similar output to what your implementation outputs. Your contribution is still valuable. I kindly ask you to modify your PR to only add the test cases.