fix: count numbered section headers in validate_sections#1767
fix: count numbered section headers in validate_sections#1767Chessing234 wants to merge 2 commits into
Conversation
Bare "Section" splits over-counted; require "Section N" like IFEvalG. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Code Review
This pull request updates the validate_sections function in open_instruct/if_functions.py to identify numbered section headers using a regular expression, ensuring that headers contain the splitter followed by a number. It also adds unit tests to verify this behavior and updates the changelog. The review feedback points out that the regular expression should be case-insensitive to support headers like SECTION 2 as described in the docstring, and suggests adding a test case to verify case-insensitive matching.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| pattern = r"\s?" + re.escape(section_splitter) + r"\s?\d+\s?" | ||
| sections = re.split(pattern, text) |
There was a problem hiding this comment.
The docstring states that headers can look like Section 1 or SECTION 2. However, without passing re.IGNORECASE to the regex engine, the matching is case-sensitive. If the section_splitter is "Section", it will fail to match SECTION 2. Compiling the pattern with re.IGNORECASE ensures that case-insensitive headers are correctly matched as described in the docstring.
pattern = r"\\s?" + re.escape(section_splitter) + r"\\s?\\d+\\s?"\n sections = re.compile(pattern, re.IGNORECASE).split(text)| ("numbered_with_intro", "Intro\nSection 1\nHello\nSection 2\nWorld", 2, "Section", True), | ||
| ("bare_splitter_not_enough", "Intro\nSection\nHello\nSection\nWorld", 2, "Section", False), | ||
| ("at_least_n", "Section 1\na\nSection 2\nb\nSection 3\nc", 2, "Section", True), | ||
| ("too_few", "Section 1\nonly one", 2, "Section", False), |
There was a problem hiding this comment.
Add a test case to verify that case-insensitive section headers (e.g., SECTION 2 when splitter is Section) are correctly validated, matching the behavior described in the docstring.
| ("numbered_with_intro", "Intro\nSection 1\nHello\nSection 2\nWorld", 2, "Section", True), | |
| ("bare_splitter_not_enough", "Intro\nSection\nHello\nSection\nWorld", 2, "Section", False), | |
| ("at_least_n", "Section 1\na\nSection 2\nb\nSection 3\nc", 2, "Section", True), | |
| ("too_few", "Section 1\nonly one", 2, "Section", False), | |
| ("numbered_with_intro", "Intro\\nSection 1\\nHello\\nSection 2\\nWorld", 2, "Section", True),\n ("case_insensitive", "Intro\\nSection 1\\nHello\\nSECTION 2\\nWorld", 2, "Section", True),\n ("bare_splitter_not_enough", "Intro\\nSection\\nHello\\nSection\\nWorld", 2, "Section", False),\n ("at_least_n", "Section 1\\na\\nSection 2\\nb\\nSection 3\\nc", 2, "Section", True),\n ("too_few", "Section 1\\nonly one", 2, "Section", False), |
Summary
validate_sectionsnow requiresSection N-style headers and>= Nsections, matching IFEvalG.Test plan