From 28ff4f00d8698a59be11050090f1293eb8e49cc3 Mon Sep 17 00:00:00 2001 From: Wyatt Childers Date: Fri, 6 Nov 2026 23:11:28 +0000 Subject: [PATCH 1/2] Add correction support to the change collector [] --- .agents/skills/edg-changes-example/SKILL.md | 40 ++- AGENTS.md | 9 +- dev_tools/bin/edg-check-file-changes | 27 +- dev_tools/bin/edg-collect-changes | 141 ++++++--- dev_tools/pylibs/edgchanges/__init__.py | 195 +++++++++++- dev_tools/tests/test_edg_collect_changes.py | 329 +++++++++++++++++++- 6 files changed, 682 insertions(+), 59 deletions(-) diff --git a/.agents/skills/edg-changes-example/SKILL.md b/.agents/skills/edg-changes-example/SKILL.md index f882316738..e17d36ee17 100644 --- a/.agents/skills/edg-changes-example/SKILL.md +++ b/.agents/skills/edg-changes-example/SKILL.md @@ -145,9 +145,43 @@ Steps: 5. Optionally preview the result without changing anything: `edg-collect-changes --dry-run src/Changes src/pending_changes`. -Edit a `Changes` file directly only to correct an entry that has already -been collected; then write it as **UTF-8** and verify the file still decodes -(raises on failure): +## Correcting an entry in the current version + +To replace entries that were already collected into the current (newest, +still open) version, write a pending entry whose first line is the +`//correction: true` directive: + +```text +//correction: true +[GH #12,GH #34] +Corrected summary + +Corrected description. +``` + +When the entry is collected, every entry in the current version that has +any of its tags is deleted, and the correction is added like any other +entry, dated the day it is collected. Its tags come from its own tag block. +The `--check` step and the pre-push hook report an error, and the nightly +job leaves the entry in place, when any of these holds: + +- One of its tags is on no entry in the current version. Corrections do not + reach across releases, so this also covers the case where the newest + version is already released. +- An entry it replaces has a tag that the correction lacks. Merge every tag + of the replaced entries into the correction's tag block. +- Another pending correction replaces the same entry. + +`--dry-run` lists the entries a correction would delete. + +A problem with an issue fixed in a prior release is usually a new issue, +not a correction. Give it its own ordinary entry, and in the description +reference the old issue and the release it was fixed in (for example, "The +fix for GH #12 in version 7.0 ..."). + +Edit a `Changes` file directly only to correct the text of an entry in a +released version. Write it as **UTF-8** and verify that the file still +decodes. The command raises on failure: ```bash python3 -c "open('path/to/Changes', encoding='utf-8').read()" diff --git a/AGENTS.md b/AGENTS.md index c21ffc886b..297f0d7863 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -100,8 +100,13 @@ words the spell check rejects (words already in the target `Changes` file pass without them). A nightly job runs `edg-collect-changes` to date them and move them into `Changes`. Check entries with `edg-exec -- edg-collect-changes --check src/Changes src/pending_changes` -(for the directory in question). Edit a `Changes` file directly -only to correct an already-collected entry. Details: skill +(for the directory in question). To replace already-collected entries +in the current (open) version, add a pending entry with a leading +`//correction: true` directive and all of the replaced entries' tags. +A problem with an issue fixed in a prior release is usually a new issue +with its own entry, whose description references the old issue and the +release it was fixed in. Edit a `Changes` file directly only to correct the +text of an entry in a released version. Details: skill **edg-changes-example**. Any `Changes` changelog in the tree (not only `src/Changes`) and every diff --git a/dev_tools/bin/edg-check-file-changes b/dev_tools/bin/edg-check-file-changes index bd022df514..29644e2fbb 100755 --- a/dev_tools/bin/edg-check-file-changes +++ b/dev_tools/bin/edg-check-file-changes @@ -244,6 +244,26 @@ def _check_overlong_lines(new_file: Path, return overlong_lines +def _check_correction(repo_path: str, entry: edgchanges.ChangesEntry + ) -> List[edgchanges.FormatError]: + '''Return the reasons the correction entry (the pending Changes entry + repo_path) can't replace entries in its Changes file.''' + # As for the spell check, the Changes file is found relative to the + # working directory. + target_path = edgchanges.target_changes_path(Path.cwd() / repo_path) + try: + with open(target_path, 'r', encoding = 'utf-8') as file_handle: + changes_lines = file_handle.read().split('\n') + except (OSError, UnicodeDecodeError) as ex: + return [ + edgchanges.FormatError( + None, + f"correction: can't read the Changes file ({ex})" + ) + ] + _, errors = edgchanges.find_corrected_entries(changes_lines, entry.tags) + return [edgchanges.FormatError(None, error) for error in errors] + def check_changes_fragment(repo_path: str, new_file: Path) -> Any: '''Check new_file, the contents of the pending Changes entry repo_path. @@ -251,12 +271,15 @@ def check_changes_fragment(repo_path: str, new_file: Path) -> Any: spelling is checked against the Changes file it will be collected into, not against an old version of the entry (see _check_fragment_spelling), and trailing whitespace (stripped when the entry is collected) doesn't count - toward the line length. + toward the line length. A correction is checked against that Changes file + too (see edgchanges.find_corrected_entries). ''' - _, format_errors = edgchanges.parse_fragment( + entry, format_errors = edgchanges.parse_fragment( new_file, check_line_length = False ) + if entry is not None and entry.is_correction: + format_errors += _check_correction(repo_path, entry) _, prose_lines = edgchanges.read_prose(new_file) return { 'format_errors': format_errors, diff --git a/dev_tools/bin/edg-collect-changes b/dev_tools/bin/edg-collect-changes index c6a8d107f6..83736d09d3 100755 --- a/dev_tools/bin/edg-collect-changes +++ b/dev_tools/bin/edg-collect-changes @@ -18,9 +18,10 @@ conflict with every other), a change adds one *.txt entry there: Previously, ... The leading //key: value directives are optional (//allowed_words: lists -words the spell check should accept). Then comes the tag block, one or more -comma-separated "GH #N" tags in brackets (it may wrap onto several lines), the -summary line, a blank line, and the description (see edgchanges). +words the spell check should accept, and "//correction: true" marks a +correction, below). Then comes the tag block, one or more comma-separated +"GH #N" tags in brackets (it may wrap onto several lines), the summary line, a +blank line, and the description (see edgchanges). Given a Changes file and its pending_changes/ directory, this tool moves every entry into the Changes file, just below the newest "Version x.y" line, @@ -30,9 +31,15 @@ released (its header has a date), a new section is opened for the next version (--version or --version-from). A nightly job runs it for each Changes file (see .github/workflows/collect-changes.yml). +A correction replaces the entries in the newest (open) version that have any +of its tags: they're deleted, and it's added like any other entry. Each of +its tags must be on such an entry, and it must have all of their tags; +otherwise (or if another correction replaces one of the same entries) it's +reported and left in place, like a malformed entry. + With --check, the entries are only checked, by running edg-check-file-changes -on them (format, spelling, typos, line length), the same check the pre-push -hook does. +on them (format, spelling, typos, line length, and whether corrections can +be applied), the same check the pre-push hook does. ''' import argparse @@ -47,20 +54,13 @@ import edgchanges import edgutil from pathlib import Path -from typing import Any, List, Optional, Tuple +from typing import Any, Dict, List, Optional, Tuple PENDING_CHANGES_KEEP_FILE_NAME = '.gitignore' SEPARATOR_LINE = '-' * edgchanges.MAX_LINE_LENGTH NO_CHANGES_LINE = 'No changes.' -# An open version is "Version 7.1"; a released one is -# "Version 7.0, September 28, 2026". -VERSION_HEADER_REGEX = re.compile( - r'^Version ([0-9]+\.[0-9]+)(, [A-Z][a-z]+ [0-9]{1,2}, [0-9]{4})?$' -) -SEPARATOR_REGEX = re.compile(r'^-+$') - def list_pending_dir(pending_dir: Path) -> Tuple[List[Path], List[Path]]: '''Return the entries in pending_dir, and the other files there (apart from the .gitignore that keeps the directory in git).''' @@ -79,31 +79,16 @@ def _describe_misnamed(path: Path) -> str: f"*{edgchanges.FRAGMENT_SUFFIX})" ) -def _find_newest_version_header(lines: List[str]) -> Optional[int]: - '''Return the index of the newest Version header line. - - Body text can also start with "Version", so a header must be the first line - of the file or directly follow a separator line. - ''' - for line_index, line in enumerate(lines): - follows_separator = ( - line_index == 0 or - SEPARATOR_REGEX.match(lines[line_index - 1]) is not None - ) - if follows_separator and VERSION_HEADER_REGEX.match(line) is not None: - return line_index - return None - def read_open_version(changes_path: Path) -> Optional[str]: '''Return the newest version of changes_path if it's still open (i.e., its header has no release date).''' with open(changes_path, 'r', encoding = 'utf-8') as file_handle: lines = file_handle.read().split('\n') - header_index = _find_newest_version_header(lines) + header_index = edgchanges.find_newest_version_header(lines) if header_index is None: return None - header_match = VERSION_HEADER_REGEX.match(lines[header_index]) + header_match = edgchanges.VERSION_HEADER_REGEX.match(lines[header_index]) assert header_match is not None if header_match.group(2) is not None: return None @@ -122,10 +107,10 @@ def insert_entries(changes_text: str, ValueError if there's nowhere to put the entries. ''' lines = changes_text.split('\n') - header_index = _find_newest_version_header(lines) + header_index = edgchanges.find_newest_version_header(lines) if header_index is None: raise ValueError('no "Version x.y" line found') - header_match = VERSION_HEADER_REGEX.match(lines[header_index]) + header_match = edgchanges.VERSION_HEADER_REGEX.match(lines[header_index]) assert header_match is not None entry_lines: List[str] = [] @@ -157,6 +142,62 @@ def insert_entries(changes_text: str, return '\n'.join(new_lines), inserted_lines +def remove_entries(changes_text: str, + removed_entries: List[edgchanges.CollectedEntry]) -> str: + '''Return changes_text without removed_entries (entries in it).''' + lines = changes_text.split('\n') + for entry in sorted(removed_entries, key = lambda entry: -entry.start): + del lines[entry.start:entry.end] + return '\n'.join(lines) + +def find_corrections(changes_text: str, + entries: List[edgchanges.ChangesEntry] + ) -> Tuple[List[edgchanges.CollectedEntry], List[str]]: + '''Find the entries in changes_text that the corrections among entries + replace. + + Returns those entries and the errors found. A correction that can't be + applied (see edgchanges.find_corrected_entries), or that replaces an entry + another one also replaces, is removed from entries. + ''' + changes_lines = changes_text.split('\n') + errors = [] + bad_entries = [] + # The corrections replacing each entry, by its first line. + correctors: Dict[int, List[edgchanges.ChangesEntry]] = {} + corrected_entries: Dict[int, edgchanges.CollectedEntry] = {} + for entry in entries: + if not entry.is_correction: + continue + corrected, correction_errors = edgchanges.find_corrected_entries( + changes_lines, + entry.tags + ) + if len(correction_errors) != 0: + errors += [f"{entry.path}: {error}" for error in correction_errors] + bad_entries.append(entry) + continue + for corrected_entry in corrected: + correctors.setdefault(corrected_entry.start, []).append(entry) + corrected_entries[corrected_entry.start] = corrected_entry + + for start, entry_correctors in correctors.items(): + if len(entry_correctors) > 1: + paths = ', '.join(str(entry.path) for entry in entry_correctors) + errors.append( + f"{paths}: corrections replacing the same entry, " + f"\"{corrected_entries[start].summary}\"" + ) + bad_entries += entry_correctors + + removed_entries = [ + corrected_entries[start] + for start, entry_correctors in correctors.items() + if not any(entry in bad_entries for entry in entry_correctors) + ] + entries[:] = [entry for entry in entries if entry not in bad_entries] + return removed_entries, errors + def _run_file_checks(fragment_paths: List[Path]) -> Any: '''Run edg-check-file-changes on fragment_paths and return its results. @@ -251,9 +292,10 @@ def collect(changes_path: Path, pending_dir: Path, date: datetime.date, next_version: Optional[str], *, dry_run: bool) -> bool: '''Move the entries in pending_dir into changes_path. - Malformed entries are reported and left in place; the others are still - collected. Returns True if every entry was collected. Raises ValueError - if changes_path has nowhere to put them. + The entries that corrections replace are deleted from changes_path. + Malformed entries, and corrections that can't be applied, are reported and + left in place; the others are still collected. Returns True if every entry + was collected. Raises ValueError if changes_path has nowhere to put them. ''' fragment_paths, other_paths = list_pending_dir(pending_dir) for path in other_paths: @@ -268,18 +310,27 @@ def collect(changes_path: Path, pending_dir: Path, date: datetime.date, ] if entry is not None: entries.append(entry) + + removed_entries: List[edgchanges.CollectedEntry] = [] + if len(entries) != 0: + with open(changes_path, 'r', encoding = 'utf-8') as file_handle: + changes_text = file_handle.read() + removed_entries, correction_errors = find_corrections( + changes_text, + entries + ) + errors += correction_errors + for error in errors: edgutil.eprint(error) if len(errors) != 0: - edgutil.eprint('Malformed entries were left in place.') + edgutil.eprint('Entries with errors were left in place.') if len(entries) == 0: return len(errors) == 0 - with open(changes_path, 'r', encoding = 'utf-8') as file_handle: - changes_text = file_handle.read() try: new_text, inserted_lines = insert_entries( - changes_text, + remove_entries(changes_text, removed_entries), entries, edgchanges.format_date(date), next_version @@ -289,6 +340,11 @@ def collect(changes_path: Path, pending_dir: Path, date: datetime.date, if dry_run: print(f"=== {changes_path}") + for removed_entry in removed_entries: + print( + f"(deleted: [{','.join(removed_entry.tags)}] " + f"{removed_entry.summary})" + ) print('\n'.join(inserted_lines)) else: # Only drop the entries once they're safely in the Changes file. @@ -297,9 +353,14 @@ def collect(changes_path: Path, pending_dir: Path, date: datetime.date, file_handle.write(new_text) for entry in entries: entry.path.unlink() + removed_text = ( + '' if len(removed_entries) == 0 else + f" (replacing {len(removed_entries)} corrected " + f"{'entry' if len(removed_entries) == 1 else 'entries'})" + ) print( f"{changes_path}: collected {len(entries)} " - f"{'entry' if len(entries) == 1 else 'entries'}" + f"{'entry' if len(entries) == 1 else 'entries'}{removed_text}" ) return len(errors) == 0 diff --git a/dev_tools/pylibs/edgchanges/__init__.py b/dev_tools/pylibs/edgchanges/__init__.py index bb1110b9d5..d70552eaf6 100644 --- a/dev_tools/pylibs/edgchanges/__init__.py +++ b/dev_tools/pylibs/edgchanges/__init__.py @@ -17,12 +17,20 @@ The leading //key: value directives (EDG C++ line directive syntax) are optional; //allowed_words: lists words the spell check should accept (beyond -those the target Changes file already uses, which it accepts anyway). Then -comes the tag block, one or more comma-separated "GH #N" tags in brackets (it -may wrap onto several lines), the summary line, a blank line, and the +those the target Changes file already uses, which it accepts anyway), and +"//correction: true" marks the entry as a correction (see below). Then comes +the tag block, one or more comma-separated "GH #N" tags in brackets (it may +wrap onto several lines), the summary line, a blank line, and the description. Lines are at most MAX_LINE_LENGTH columns, not counting trailing whitespace (which is stripped when the entry is collected). +A correction replaces entries already collected into the current (newest, +still open) version of its Changes file: when it's collected, every entry +there with one of its tags is deleted, and it's added like any other entry +(dated the day it's collected). Corrections don't reach across releases, so +each of its tags must be on an entry in the current version, and every tag of +those entries must be on the correction (see find_corrected_entries). + The format is checked by edg-check-file-changes (which the pre-push hook and edg-collect-changes --check run) and relied on by edg-collect-changes. ''' @@ -37,6 +45,8 @@ FRAGMENT_SUFFIX = '.txt' ALLOWED_WORDS_DIRECTIVE = 'allowed_words' +CORRECTION_DIRECTIVE = 'correction' +CORRECTION_DIRECTIVE_VALUE = 'true' PENDING_CHANGES_DIR_NAME = 'pending_changes' CHANGES_FILE_NAME = 'Changes' @@ -47,6 +57,23 @@ TAG_REGEX = re.compile(r'^GH #[0-9]+$') +# An open version is "Version 7.1"; a released one is +# "Version 7.0, September 28, 2026". +VERSION_HEADER_REGEX = re.compile( + r'^Version ([0-9]+\.[0-9]+)(, [A-Z][a-z]+ [0-9]{1,2}, [0-9]{4})?$' +) +SEPARATOR_REGEX = re.compile(r'^-+$') + +# The first line of an entry in a Changes file: the date and the start of the +# tag block. +_COLLECTED_ENTRY_START_REGEX = re.compile( + r'^[0-9]{1,2}/[0-9]{1,2}/[0-9]{2} +\[' +) + +_CORRECTION_DIRECTIVE_LINE_REGEX = re.compile( + r'^\s*//' + CORRECTION_DIRECTIVE + ':' +) + # The internal issue tracker form of a tag; accepted in commit messages but # not in Changes entries. EDG_TAG_REGEX = re.compile(r'^EDG[cfjp]+fe/[0-9]+$') @@ -163,11 +190,12 @@ class ChangesEntry: '''A parsed pending Changes entry.''' def __init__(self, path: Path, tags: List[str], summary: str, - body_lines: List[str]) -> None: + body_lines: List[str], *, is_correction: bool = False) -> None: self.path = path self.tags = tags self.summary = summary self.body_lines = body_lines + self.is_correction = is_correction def sort_key(self) -> Tuple[str, str, str]: return (self.summary.casefold(), self.summary, self.path.name) @@ -187,6 +215,37 @@ def _find_tag_block_end(lines: List[str]) -> Optional[int]: None ) +def _split_tags(tag_block_lines: List[str]) -> List[str]: + '''Return the tags in the tag block on tag_block_lines (anything before its + '[' is ignored).''' + tag_text = ' '.join(line.strip() for line in tag_block_lines) + tag_text = tag_text[tag_text.index('[') + 1:tag_text.index(']')] + return [tag.strip() for tag in tag_text.split(',')] + +def _check_correction_directive(lines: List[str], + values: List[str]) -> List[FormatError]: + '''Return the problems with an entry's //correction: directives (whose + values are values), given the lines of the entry.''' + line_nos = [ + line_no for line_no, line in enumerate(lines, start = 1) + if _CORRECTION_DIRECTIVE_LINE_REGEX.match(line) is not None + ] + errors = [] + for line_no, value in zip(line_nos, values): + if value.strip() != CORRECTION_DIRECTIVE_VALUE: + errors.append( + FormatError( + line_no, + f"invalid //{CORRECTION_DIRECTIVE}: value '{value.strip()}'; use " + f"'//{CORRECTION_DIRECTIVE}: {CORRECTION_DIRECTIVE_VALUE}'" + ) + ) + for line_no in line_nos[1:]: + errors.append( + FormatError(line_no, f"//{CORRECTION_DIRECTIVE}: is repeated") + ) + return errors + def parse_fragment(path: Path, *, check_line_length: bool = True ) -> Tuple[Optional[ChangesEntry], List[FormatError]]: '''Parse the pending Changes entry at path. @@ -211,7 +270,14 @@ def parse_fragment(path: Path, *, check_line_length: bool = True FormatError(line_no, f"line longer than {MAX_LINE_LENGTH} columns") ) - _, remaining_lines = edgutil.split_leading_cpp_line_directives(lines) + directive_map, remaining_lines = ( + edgutil.split_leading_cpp_line_directives(lines) + ) + correction_values = directive_map.get(CORRECTION_DIRECTIVE, []) + errors += _check_correction_directive( + lines[:len(lines) - len(remaining_lines)], + correction_values + ) first_line_no = len(lines) - len(remaining_lines) + 1 remaining_lines = [line.rstrip() for line in remaining_lines] while remaining_lines and remaining_lines[-1] == '': @@ -236,10 +302,7 @@ def parse_fragment(path: Path, *, check_line_length: bool = True ) return None, errors - tag_text = ' '.join( - line.strip() for line in remaining_lines[:close_index + 1] - )[1:-1] - tags = [tag.strip() for tag in tag_text.split(',')] + tags = _split_tags(remaining_lines[:close_index + 1]) if tags == ['']: errors.append( FormatError(first_line_no, "empty tag block; use e.g. '[GH #123]'") @@ -283,9 +346,121 @@ def parse_fragment(path: Path, *, check_line_length: bool = True path, tags, remaining_lines[summary_index], - remaining_lines[summary_index + 2:] + remaining_lines[summary_index + 2:], + is_correction = len(correction_values) != 0 ), [] +def find_newest_version_header(lines: List[str]) -> Optional[int]: + '''Return the index of the newest Version header line of a Changes file. + + Body text can also start with "Version", so a header must be the first line + of the file or directly follow a separator line. + ''' + for line_index, line in enumerate(lines): + follows_separator = ( + line_index == 0 or + SEPARATOR_REGEX.match(lines[line_index - 1]) is not None + ) + if follows_separator and VERSION_HEADER_REGEX.match(line) is not None: + return line_index + return None + +# An entry in a Changes file: its lines are [start, end) (including the blank +# lines after it). +CollectedEntry = NamedTuple( + 'CollectedEntry', + [('start', int), ('end', int), ('tags', List[str]), ('summary', str)] +) + +def read_current_entries(lines: List[str]) -> Optional[List[CollectedEntry]]: + '''Return the entries in the newest version of the Changes file whose lines + are lines, or None if that version is released (or there is none).''' + header_index = find_newest_version_header(lines) + if header_index is None: + return None + header_match = VERSION_HEADER_REGEX.match(lines[header_index]) + assert header_match is not None + if header_match.group(2) is not None: + return None + + # The section ends at the separator before the next Version header. + section_end = next( + ( + index for index in range(header_index + 1, len(lines) - 1) + if SEPARATOR_REGEX.match(lines[index]) is not None and + VERSION_HEADER_REGEX.match(lines[index + 1]) is not None + ), + len(lines) + ) + # Entries are normally separated by two blank lines, but not always (a + # hand edit may have lost them), so only the first line is relied on. + starts = [ + index for index in range(header_index + 1, section_end) + if _COLLECTED_ENTRY_START_REGEX.match(lines[index]) is not None + ] + + entries = [] + for start, end in zip(starts, starts[1:] + [section_end]): + close_index = _find_tag_block_end(lines[start:end]) + if close_index is None: + continue + summary_index = start + close_index + 1 + entries.append( + CollectedEntry( + start, + end, + _split_tags(lines[start:summary_index]), + lines[summary_index] if summary_index < end else '' + ) + ) + return entries + +# Added to the errors for a correction naming a tag the current version +# doesn't have, which is often one fixed in a prior release. +_PRIOR_RELEASE_HINT = ( + 'correction: corrections can\'t reach across releases; a problem with an ' + 'issue fixed in a prior release is usually a new issue, with an ordinary ' + 'entry that references the old issue and the release it was fixed in' +) + +def find_corrected_entries(changes_lines: List[str], tags: List[str] + ) -> Tuple[List[CollectedEntry], List[str]]: + '''Return the entries a correction with tags replaces in the Changes file + whose lines are changes_lines, and the reasons it can't (if any). + + It replaces the entries in the newest version with any of its tags. Each + of its tags must be on one of them (corrections don't reach across + releases), and it must have all of their tags. + ''' + current_entries = read_current_entries(changes_lines) + if current_entries is None: + return [], [ + 'a correction must replace entries in the current version, but the ' + 'newest version of the Changes file is released', + _PRIOR_RELEASE_HINT + ] + + tag_set = set(tags) + corrected_entries = [ + entry for entry in current_entries if not tag_set.isdisjoint(entry.tags) + ] + corrected_tags = {tag for entry in corrected_entries for tag in entry.tags} + errors = [ + f"correction: no entry in the current version has the tag '{tag}'" + for tag in tags if tag not in corrected_tags + ] + if len(errors) != 0: + errors.append(_PRIOR_RELEASE_HINT) + for entry in corrected_entries: + missing_tags = [tag for tag in entry.tags if tag not in tag_set] + if len(missing_tags) != 0: + errors.append( + f"correction: the entry it replaces, \"{entry.summary}\", also has " + f"{', '.join(repr(tag) for tag in missing_tags)}; add " + f"{'it' if len(missing_tags) == 1 else 'them'} to the tag block" + ) + return corrected_entries, errors + def read_prose(path: Path) -> Tuple[Set[str], List[str]]: '''Return the words an entry's //allowed_words: directives accept, and the lines of the entry to spell check (everything after the directives and the diff --git a/dev_tools/tests/test_edg_collect_changes.py b/dev_tools/tests/test_edg_collect_changes.py index c22b0988f1..c7b846cf2e 100644 --- a/dev_tools/tests/test_edg_collect_changes.py +++ b/dev_tools/tests/test_edg_collect_changes.py @@ -85,6 +85,40 @@ def _load_collect_tool() -> Any: def _fragment(tags: str, summary: str, body: str = 'Body.') -> str: return f"{tags}\n{summary}\n\n{body}\n" +def _correction(tags: str, summary: str) -> str: + return '//correction: true\n' + _fragment(tags, summary) + +WRAPPED_TAGS = [f"GH #{number}" for number in range(4, 15)] + +# The current version (7.1) has three entries, the last with a wrapped tag +# block; GH #20 is only in the released version. +CORRECTABLE_HEADER = 'Version 7.1\n\n' +CORRECTABLE_FIRST = '10/20/26 [GH #1]\nFirst\n\nFirst body.\n\n\n' +CORRECTABLE_SECOND = ( + '10/19/26 [GH #2,GH #3]\nSecond\n\nSecond body.\n\n Example.\n\n\n' +) +CORRECTABLE_WRAPPED = ( + '\n'.join(edgchanges.format_tag_lines('10/18/26', WRAPPED_TAGS)) + + '\nWrapped\n\nWrapped body.\n\n\n' +) +CORRECTABLE_RELEASED = ( + f"{'-' * 79}\n" + 'Version 7.0, September 28, 2026\n' + '\n' + '9/1/26 [GH #20]\n' + 'Released entry\n' + '\n' + 'Released body.\n' +) +CORRECTABLE_CHANGES = ( + CORRECTABLE_HEADER + CORRECTABLE_FIRST + CORRECTABLE_SECOND + + CORRECTABLE_WRAPPED + CORRECTABLE_RELEASED +) + +def _collected(tags: str, summary: str) -> str: + '''Return a _fragment(tags, summary) entry as collected on 10/21/26.''' + return f"10/21/26 {tags}\n{summary}\n\nBody.\n\n\n" + class FakeTree: '''A scratch tree of Changes files and pending_changes/ directories.''' @@ -405,11 +439,11 @@ def test_header_needs_separator(self) -> None: # line that doesn't follow a separator. lines = OPEN_CHANGES.split('\n') self.assertEqual( - collect_tool._find_newest_version_header(lines[2:] + lines[:2]), + edgchanges.find_newest_version_header(lines[2:] + lines[:2]), lines[2:].index('Version 7.0, September 28, 2026') ) self.assertIsNone( - collect_tool._find_newest_version_header(['', 'Version 7.1']) + edgchanges.find_newest_version_header(['', 'Version 7.1']) ) def test_sort_tie_break(self) -> None: @@ -476,6 +510,255 @@ def test_check_rejects_misnamed_file(self) -> None: ) self.assertIn('notes.md: not a pending Changes entry', output.getvalue()) +class CorrectionTests(TreeTestCase): + + def _collect(self, *fragments: str, dry_run: bool = False) -> bool: + self.changes_path = self.tree.add_changes('src', CORRECTABLE_CHANGES) + self.fragment_paths = [ + self.tree.add_fragment('src', f"f{index}.txt", text) + for index, text in enumerate(fragments) + ] + return self.tree.collect(dry_run = dry_run) + + def _changes_text(self) -> str: + return self.changes_path.read_text(encoding = 'utf-8') + + def test_parse(self) -> None: + root = self.tree.root + root.joinpath('a.txt').write_text(_correction('[GH #1]', 'A')) + entry, errors = edgchanges.parse_fragment(root / 'a.txt') + self.assertEqual(errors, []) + self.assertTrue(entry.is_correction) + + root.joinpath('b.txt').write_text(_fragment('[GH #1]', 'B')) + entry, errors = edgchanges.parse_fragment(root / 'b.txt') + self.assertFalse(entry.is_correction) + + root.joinpath('c.txt').write_text( + '//allowed_words: x\n//correction: false\n' + + _fragment('[GH #1]', 'C') + ) + entry, errors = edgchanges.parse_fragment(root / 'c.txt') + self.assertIsNone(entry) + self.assertEqual( + errors, + [ + edgchanges.FormatError( + 2, + "invalid //correction: value 'false'; use '//correction: true'" + ) + ] + ) + + root.joinpath('d.txt').write_text( + '//correction: true\n' + _correction('[GH #1]', 'D') + ) + entry, errors = edgchanges.parse_fragment(root / 'd.txt') + self.assertIsNone(entry) + self.assertEqual( + errors, + [edgchanges.FormatError(2, '//correction: is repeated')] + ) + + def test_read_current_entries(self) -> None: + lines = CORRECTABLE_CHANGES.split('\n') + entries = edgchanges.read_current_entries(lines) + assert entries is not None + self.assertEqual( + [(entry.tags, entry.summary) for entry in entries], + [ + (['GH #1'], 'First'), + (['GH #2', 'GH #3'], 'Second'), + (WRAPPED_TAGS, 'Wrapped') + ] + ) + self.assertIsNone( + edgchanges.read_current_entries(RELEASED_CHANGES.split('\n')) + ) + + # An open version with no entries (as just after a release) has none; + # the released version's entries don't count. + empty_open_lines = ( + f"Version 7.2\n\nNo changes.\n\n\n{'-' * 79}\n" + + CORRECTABLE_CHANGES.replace( + 'Version 7.1\n', + 'Version 7.1, November 1, 2026\n' + ) + ).split('\n') + self.assertEqual(edgchanges.read_current_entries(empty_open_lines), []) + + # Entries are found by their dated first lines alone, so two missing + # the blank lines between them are still told apart. Body lines that + # look like a Version header or a separator don't end the section. + tricky_lines = ( + 'Version 7.1\n' + '\n' + '10/20/26 [GH #1]\n' + 'First\n' + '\n' + 'Version 7.0 of the front end did this.\n' + '10/19/26 [GH #2]\n' + 'Second\n' + '\n' + f"{'-' * 20}\n" + 'Not a header.\n' + '\n' + '\n' + + CORRECTABLE_RELEASED + ).split('\n') + entries = edgchanges.read_current_entries(tricky_lines) + assert entries is not None + self.assertEqual( + [(entry.start, entry.end, entry.tags, entry.summary) + for entry in entries], + [(2, 6, ['GH #1'], 'First'), (6, 13, ['GH #2'], 'Second')] + ) + self.assertEqual( + edgchanges.find_corrected_entries(tricky_lines, ['GH #1']), + ([entries[0]], []) + ) + + def test_replace_one(self) -> None: + self.assertTrue(self._collect(_correction('[GH #1]', 'New first'))) + self.assertEqual( + self._changes_text(), + CORRECTABLE_HEADER + _collected('[GH #1]', 'New first') + + CORRECTABLE_SECOND + CORRECTABLE_WRAPPED + CORRECTABLE_RELEASED + ) + self.assertFalse(self.fragment_paths[0].exists()) + + def test_replace_several(self) -> None: + # One correction can merge several entries; other entries are collected + # as usual alongside it. + self.assertTrue( + self._collect( + _correction('[GH #3,GH #1,GH #2]', 'Merged'), + _fragment('[GH #30]', 'Unrelated') + ) + ) + self.assertEqual( + self._changes_text(), + CORRECTABLE_HEADER + _collected('[GH #3,GH #1,GH #2]', 'Merged') + + _collected('[GH #30]', 'Unrelated') + CORRECTABLE_WRAPPED + + CORRECTABLE_RELEASED + ) + + def test_replace_wrapped_last(self) -> None: + # The last entry before the separator, with a wrapped tag block. + tags = '[' + ',\n '.join(WRAPPED_TAGS) + ']' + self.assertTrue(self._collect(_correction(tags, 'New wrapped'))) + self.assertEqual( + self._changes_text(), + CORRECTABLE_HEADER + + '\n'.join(edgchanges.format_tag_lines('10/21/26', WRAPPED_TAGS)) + + '\nNew wrapped\n\nBody.\n\n\n' + + CORRECTABLE_FIRST + CORRECTABLE_SECOND + CORRECTABLE_RELEASED + ) + + def test_replace_all(self) -> None: + tags = '[' + ',\n '.join(['GH #1', 'GH #2', 'GH #3'] + WRAPPED_TAGS) + ']' + self.assertTrue(self._collect(_correction(tags, 'Everything'))) + text = self._changes_text() + self.assertTrue(text.startswith(CORRECTABLE_HEADER + '10/21/26 [GH #1,')) + self.assertTrue( + text.endswith('\nEverything\n\nBody.\n\n\n' + CORRECTABLE_RELEASED) + ) + + def _assert_rejected(self, expected_error: str, *fragments: str) -> None: + with contextlib.redirect_stderr(io.StringIO()) as output: + changes_path = self.tree.add_changes('src', CORRECTABLE_CHANGES) + for index, text in enumerate(fragments): + self.tree.add_fragment('src', f"f{index}.txt", text) + succeeded = collect_tool.collect( + changes_path, + self.tree.root / 'src' / 'pending_changes', + datetime.date(2026, 10, 21), + None, + dry_run = False + ) + self.assertFalse(succeeded) + self.assertIn(expected_error, output.getvalue()) + self.assertEqual( + changes_path.read_text(encoding = 'utf-8'), + CORRECTABLE_CHANGES + ) + for index in range(len(fragments)): + self.assertTrue( + (self.tree.root / 'src' / 'pending_changes' / f"f{index}.txt") + .exists() + ) + shutil.rmtree(self.tree.root / 'src') + + def test_errors(self) -> None: + self._assert_rejected( + "no entry in the current version has the tag 'GH #20'", + _correction('[GH #20]', 'Released') + ) + self._assert_rejected( + 'issue fixed in a prior release is usually a new issue', + _correction('[GH #20]', 'Released') + ) + self._assert_rejected( + "no entry in the current version has the tag 'GH #99'", + _correction('[GH #1,GH #99]', 'Unknown') + ) + self._assert_rejected( + 'the entry it replaces, "Second", also has \'GH #3\'; add it', + _correction('[GH #2]', 'Partial') + ) + self._assert_rejected( + 'corrections replacing the same entry, "First"', + _correction('[GH #1]', 'One'), + _correction('[GH #1]', 'Two') + ) + + def test_bad_correction_left_in_place(self) -> None: + # A bad correction doesn't keep the other entries from being collected. + self.assertFalse( + self._collect( + _correction('[GH #2]', 'Partial'), + _fragment('[GH #30]', 'Unrelated') + ) + ) + self.assertTrue(self.fragment_paths[0].exists()) + self.assertFalse(self.fragment_paths[1].exists()) + self.assertEqual( + self._changes_text(), + CORRECTABLE_HEADER + _collected('[GH #30]', 'Unrelated') + + CORRECTABLE_CHANGES[len(CORRECTABLE_HEADER):] + ) + + def test_released_version(self) -> None: + changes_path = self.tree.add_changes('lib_src', RELEASED_CHANGES) + fragment = self.tree.add_fragment( + 'lib_src', 'a.txt', _correction('[GH #1]', 'A') + ) + self.assertFalse(self.tree.collect('lib_src', version = '7.1')) + self.assertTrue(fragment.exists()) + self.assertEqual( + changes_path.read_text(encoding = 'utf-8'), + RELEASED_CHANGES + ) + + def test_dry_run(self) -> None: + with contextlib.redirect_stdout(io.StringIO()) as output: + self.changes_path = self.tree.add_changes('src', CORRECTABLE_CHANGES) + fragment = self.tree.add_fragment( + 'src', 'a.txt', _correction('[GH #2,GH #3]', 'New second') + ) + self.assertTrue( + collect_tool.collect( + self.changes_path, + self.tree.root / 'src' / 'pending_changes', + datetime.date(2026, 10, 21), + None, + dry_run = True + ) + ) + self.assertIn('(deleted: [GH #2,GH #3] Second)', output.getvalue()) + self.assertTrue(fragment.exists()) + self.assertEqual(self._changes_text(), CORRECTABLE_CHANGES) + class FragmentPathTests(unittest.TestCase): def test_is_fragment_path(self) -> None: @@ -584,6 +867,48 @@ def test_trailing_whitespace_not_overlong(self) -> None: _fragment('[GH #1]', 'Summary', 'x' * 80)) self.assertEqual(list(results['overlong_lines']), ['4']) + def test_correction(self) -> None: + (self.tmp_dir / 'src').mkdir() + (self.tmp_dir / 'src' / 'Changes').write_text(CORRECTABLE_CHANGES) + + results = self._check( + 'src/pending_changes/a.txt', + _correction('[GH #2,GH #3]', 'Summary') + ) + self.assertEqual(results['format_errors'], []) + + results = self._check( + 'src/pending_changes/a.txt', + _correction('[GH #20]', 'Summary') + ) + self.assertEqual( + results['format_errors'], + [ + [None, "correction: no entry in the current version has the tag " + "'GH #20'"], + [None, edgchanges._PRIOR_RELEASE_HINT] + ] + ) + + # Without the directive, the tags aren't checked against the Changes + # file. + results = self._check( + 'src/pending_changes/a.txt', + _fragment('[GH #20]', 'Summary') + ) + self.assertEqual(results['format_errors'], []) + + # The target Changes file has to exist. + results = self._check( + 'util/pending_changes/a.txt', + _correction('[GH #1]', 'Summary') + ) + self.assertEqual(len(results['format_errors']), 1) + self.assertIn( + "can't read the Changes file", + results['format_errors'][0][1] + ) + @unittest.skipUnless(_aspell_works(), 'aspell is not available') def test_allowed_words(self) -> None: text = _fragment('[GH #1,\n GH #2]', 'Summary', 'A frobnicator lvalue.') From 4e20dec95291f0549bc23a32bf312a665ee1ab60 Mon Sep 17 00:00:00 2001 From: Wyatt Childers Date: Fri, 6 Nov 2026 23:59:15 +0000 Subject: [PATCH 2/2] Fix Changes formatting issue [] --- src/Changes | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Changes b/src/Changes index f8c9d33a98..47949a8f44 100644 --- a/src/Changes +++ b/src/Changes @@ -869,6 +869,8 @@ C++-generating back end: inaccessible or out-of-scope template arguments In certain complex cases, the C++-generating back end could put out a template argument that uses an inaccessible or out-of-scope name, making the generated code uncompilable. This is now fixed. + + 10/6/26 [GH #203] Assertion failure on an empty diag_suppress pragma