-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Prevent page brake in the middle of a seealso
#8519
Conversation
@jfbu Could you review this please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for contribution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits.
sphinx/writers/latex.py
Outdated
@@ -813,7 +813,7 @@ def depart_desc_content(self, node: Element) -> None: | |||
pass | |||
|
|||
def visit_seealso(self, node: Element) -> None: | |||
self.body.append('\n\n\\sphinxstrong{%s:}\n\n' % admonitionlabels['seealso']) | |||
self.body.append('\n\n\\sphinxstrong{%s:}\n\\nopagebreak\n\n' % admonitionlabels['seealso']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
./sphinx/writers/latex.py:816:96: E501 line too long (100 > 95 characters)
https://github.com/sphinx-doc/sphinx/pull/8519/checks?check_run_id=1500490152
Could you fold this line, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tk0miya I'm actually for the 79 char line length limit more than most people, but in this case IMHO it is irrelevant as the rest of the file has many exceptions (in many cases much more lengthier than this). Keeping it on one line made the diff more clear as it only contains the relevant change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like to discuss the coding rule of this project to merge this change? If so, please file another issue first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I will not insist if the coding guidelines are not flexible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for update. I'll merge this soon after tests and linting passed.
Merged! |
Oops... this change was merged to master branch unexpectedly... I'll cherry-pick this manually later. |
Subject: Prevent page brake in the middle of a
seealso
Feature or Bugfix
Purpose
Detail
seealso
directives usually contain short content. It looks bad when the "See also:" strong text is alone at the end of a page and the associated content is at the top of the next one.