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

Prevent page brake in the middle of a seealso  #8519

Merged
merged 2 commits into from Dec 8, 2020

Conversation

bosonogi
Copy link
Contributor

@bosonogi bosonogi commented Dec 4, 2020

Subject: Prevent page brake in the middle of a seealso

Feature or Bugfix

  • Bugfix

Purpose

  • Minor change to improve the quality of latex output.

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.

@tk0miya
Copy link
Member

tk0miya commented Dec 6, 2020

@jfbu Could you review this please?

Copy link
Contributor

@jfbu jfbu left a 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

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

@@ -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'])
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

@tk0miya tk0miya merged commit 28c99cb into sphinx-doc:master Dec 8, 2020
@tk0miya
Copy link
Member

tk0miya commented Dec 8, 2020

Merged!

@tk0miya
Copy link
Member

tk0miya commented Dec 8, 2020

Oops... this change was merged to master branch unexpectedly... I'll cherry-pick this manually later.

tk0miya added a commit that referenced this pull request Dec 8, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants