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

Support glossary directive #10

Merged
merged 10 commits into from
Jan 16, 2024
Merged

Conversation

adam-zlatniczki
Copy link

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

coveralls commented Nov 8, 2023

Pull Request Test Coverage Report for Build 7539248394

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 100.0%

Totals Coverage Status
Change from base Build 7179832233: 0.3%
Covered Lines: 721
Relevant Lines: 721

💛 - Coveralls

@liran-funaro
Copy link
Owner

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

adam-zlatniczki commented Dec 3, 2023 via email

- all files are now black compliant
- duplicate empty lines removed from glossaries.md
- Pylint gave irrelevant warnings, they got disabled
- Synchronized glossaries md and rst
- Synchronized blocks rst and md
- Moved pylint rule disable comment
@adam-zlatniczki
Copy link
Author

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

liran-funaro commented Dec 11, 2023

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.

@liran-funaro liran-funaro added the enhancement New feature or request label Dec 11, 2023
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>

# Conflicts:
#	sphinx_markdown_builder/translator.py
@liran-funaro liran-funaro merged commit 0cde2db into liran-funaro:main Jan 16, 2024
7 checks passed
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.

None yet

3 participants