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

#8904: value.split(';', n - 1) -> value.split('; ', n - 1) #9751

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KaKkouo
Copy link

@KaKkouo KaKkouo commented Oct 18, 2021

Subject: #8904 fix about split_into function.

Feature or Bugfix

  • Bugfix

Purpose

  • For value.split used in the split_into function, the argument has been modified to properly separate the terms.
  • Since this is a modification of the term delimiters in the index directive, there is no particular environment or extension that it depends on.

Detail

about syntax index directive

  • A semicolon is followed by a space.
  • Therefore, changing the argument of the value.split function from ";" to "; "+space is no problem. In fact, it is appropriate to change it.
.. index:: term1; term2

source code

  • sphinx/util/init.py
    • The target of this revision.
  • sphinx/environment/adapters/indexentries.py
  • IndexEntries class, create_index method
  • sphinx/writers/latex.py
    • LaTeXTranslator class, visit_index method
      • split_into is being used.
      • The fix for split_into should not be a problem since it is a read-dependent change, but I have not checked it.

Relates

@KaKkouo
Copy link
Author

KaKkouo commented Oct 18, 2021

  • I've seen the circleci FAIL, and I know from test_html_index_entries that it's a test related to split_into, but I can't figure out why I'm getting the "MAILING LIST" error.
  • This situation is not explained in the Contributing to Sphinx section, should I just wait?

@tk0miya
Copy link
Member

tk0miya commented Oct 20, 2021

Thank you for your contribution.

At present, term1;term2 (without whitespace) is recognized as two parts. In other words, it's a valid definition. Therefore, this fix brings a breaking change too.

@KaKkouo
Copy link
Author

KaKkouo commented Oct 23, 2021

It turned out that the proposed changes were the ones that would break compatibility. Thanks for letting me know. Also, I apologize for the hassle.

@KaKkouo
Copy link
Author

KaKkouo commented Oct 23, 2021

I would like to discuss how to handle #8904. Should I just close it with "not a bug"? Or should I leave it to the Sphinx members?

If it is better for me to handle it, I will explain the following points:

  • It's not a bug.
  • He can fix the code or create a Sphinx extension to use it in his project.
  • Or, he have to create sphinx/domains/prolog.py

@tk0miya tk0miya changed the base branch from 4.x to 5.x May 22, 2022 12:57
@AA-Turner AA-Turner added the type:enhancement enhance or introduce a new feature label May 23, 2022
@AA-Turner
Copy link
Member

Marking this as an "enhancement" -- the way forward here would be to detect ; (no space) and raise a warning noting that in Sphinx 7.0 we would change to require ; (with a space). Then the prolog use-case would work.

A complementary approach would be an opt-in configuration variable to require ; (with space) now, allowing the prolog use-case today rather than when Sphinx 7.0 is released.

A

@AA-Turner AA-Turner changed the base branch from 5.x to master October 16, 2022 15:24
@AA-Turner AA-Turner added this to the some future version milestone Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants