Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix: Abstract models should not show 'database table' string in rendered documentation #40

Merged
merged 1 commit into from
Jul 2, 2023
Merged

Conversation

insspb
Copy link
Contributor

@insspb insspb commented Jun 12, 2023

This fixes issue, raised by me in #39

It also rename database model related options, and add few other.

fixes #39

@insspb
Copy link
Contributor Author

insspb commented Jun 12, 2023

@timoludwig, please take a look. I am plan to add few more pull request on things, that bothers me.

But as all of them related to model generation, I do not want to add all at once, to exclude merge conflicts.

@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2023

Codecov Report

Merging #40 (89edbac) into main (955853e) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 89edbac differs from pull request most recent head 2f0a76b. Consider uploading reports for the commit 2f0a76b to get more accurate results

@@            Coverage Diff            @@
##              main       #40   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          308       314    +6     
=========================================
+ Hits           308       314    +6     
Impacted Files Coverage Δ
sphinxcontrib_django/docstrings/__init__.py 100.00% <100.00%> (ø)
sphinxcontrib_django/docstrings/classes.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @insspb,
Thanks a lot for your support and your motivation to add more contributions in the future 💪

I agree with the point of the issue and like the solution, however I would prefer a simpler approach and think the custom placeholders are not really necessary, if that's ok with you?

README.rst Outdated Show resolved Hide resolved
@insspb
Copy link
Contributor Author

insspb commented Jun 14, 2023

@timoludwig I removed django_tables_names_abstract_name option.
Second PR (#42) fixed too.

Co-authored-by: Timo Ludwig <ti.ludwig@web.de>
Copy link
Collaborator

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@insspb Thanks a lot for your changes and sorry for the delay!

I reverted your renaming of the config variable to allow backward compatibility and added a changelog entry, but other than that it looks good! 👍

@timobrembeck timobrembeck merged commit db7325a into sphinx-doc:main Jul 2, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abstract models should not show 'database table' string in rendered documentation
3 participants