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

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
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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