diff --git a/py/envoy.code.check/envoy/code/check/abstract/changelog.py b/py/envoy.code.check/envoy/code/check/abstract/changelog.py index e013db6597..c2012f0a52 100644 --- a/py/envoy.code.check/envoy/code/check/abstract/changelog.py +++ b/py/envoy.code.check/envoy/code/check/abstract/changelog.py @@ -22,6 +22,8 @@ MAX_VERSION_FOR_CHANGES_SECTION = "1.16" +# YAML block scalars add four spaces to Envoy's 140-character limit. +MAX_CHANGELOG_ENTRY_LINE_LENGTH = 136 VALID_CHANGELOG_AREA_RE = re.compile(r"^[a-z0-9_\-/]+$") VALID_CHANGELOG_AREA_PATTERN = r"[a-z0-9_\-/]+" @@ -174,6 +176,18 @@ def check_entry_content( f"{path}: Entry file is empty or contains only whitespace") return None + def check_entry_line_lengths( + self, + path: pathlib.Path) -> tuple[str, ...]: + return tuple( + f"{path}:{line_number}: Changelog entry line is " + f"{len(line)} characters (maximum " + f"{MAX_CHANGELOG_ENTRY_LINE_LENGTH})" + for line_number, line + in enumerate(path.read_text().splitlines(), start=1) + if (len(line) > MAX_CHANGELOG_ENTRY_LINE_LENGTH + and len(line.split()) > 1)) + def check_entry_files( self, paths: list[pathlib.Path]) -> tuple[str, ...]: @@ -183,6 +197,7 @@ def check_entry_files( errors.append(err) if err := self.check_entry_content(path): errors.append(err) + errors.extend(self.check_entry_line_lengths(path)) return tuple(errors) @@ -231,10 +246,7 @@ def duplicate_current(self) -> bool: def entry_dir(self) -> pathlib.Path | None: if not self.is_current: return None - return ( - self.project.changelogs - .changelog_path(self.version) - .with_suffix("")) + return self.project.changelogs.current_dir_path @async_property(cache=True) async def errors(self) -> tuple[str, ...]: @@ -305,7 +317,7 @@ async def check_date(self) -> tuple[str, ...]: # In the entries layout the current changelog has no real date; it is # synthesized as `Pending` until `write_version` bakes a dated file, # so there is nothing to validate here. - if self.is_current and self.project.changelogs.entries_layout: + if self.is_current: return () errors = [] if invalid_date := await self.invalid_date: diff --git a/py/envoy.code.check/tests/test_abstract_changelog.py b/py/envoy.code.check/tests/test_abstract_changelog.py index 1068af3587..ad56f1d05f 100644 --- a/py/envoy.code.check/tests/test_abstract_changelog.py +++ b/py/envoy.code.check/tests/test_abstract_changelog.py @@ -1030,31 +1030,66 @@ def test_changeschecker_check_entry_content(content, expected): assert substring in result +@pytest.mark.parametrize( + "content,expected", + [(f"{'a' * 134} b", ()), + ("a" * 137, ()), + (f"{'a' * 135} b", + ("entry.rst:1: Changelog entry line is 137 characters " + "(maximum 136)", )), + (f"valid\n{'a' * 135} b\n{'a' * 136} c", + ("entry.rst:2: Changelog entry line is 137 characters " + "(maximum 136)", + "entry.rst:3: Changelog entry line is 138 characters " + "(maximum 136)"))]) +def test_changeschecker_check_entry_line_lengths( + tmp_path, content, expected): + changelog = DummyChangelogChangesChecker("SECTIONS") + path = tmp_path / "entry.rst" + path.write_text(content) + + assert ( + changelog.check_entry_line_lengths(path) + == tuple(f"{tmp_path}/{error}" for error in expected)) + + def test_changeschecker_check_entry_files(patches): changelog = DummyChangelogChangesChecker("SECTIONS") patched = patches( "AChangelogChangesChecker.check_entry_content", "AChangelogChangesChecker.check_entry_filename", + "AChangelogChangesChecker.check_entry_line_lengths", prefix="envoy.code.check.abstract.changelog") paths = [MagicMock(), MagicMock(), MagicMock()] - # path 0: filename error + content error + # path 0: filename error + content error + line length error # path 1: no errors - # path 2: content error only + # path 2: content error + line length errors filename_returns = ["FILENAME_ERR", None, None] content_returns = ["CONTENT_ERR", None, "CONTENT_ERR2"] + line_length_returns = [ + ("LINE_LENGTH_ERR", ), + (), + ("LINE_LENGTH_ERR2", "LINE_LENGTH_ERR3")] - with patched as (m_content, m_filename): + with patched as (m_content, m_filename, m_line_lengths): m_filename.side_effect = filename_returns m_content.side_effect = content_returns + m_line_lengths.side_effect = line_length_returns result = changelog.check_entry_files(paths) - assert result == ("FILENAME_ERR", "CONTENT_ERR", "CONTENT_ERR2") + assert ( + result + == ("FILENAME_ERR", "CONTENT_ERR", "LINE_LENGTH_ERR", + "CONTENT_ERR2", "LINE_LENGTH_ERR2", "LINE_LENGTH_ERR3")) assert ( m_filename.call_args_list == [[(p, ), {}] for p in paths]) assert ( m_content.call_args_list == [[(p, ), {}] for p in paths]) + assert ( + m_line_lengths.call_args_list + == [[(p, ), {}] for p in paths]) @pytest.mark.parametrize("is_current", [True, False]) @@ -1065,32 +1100,19 @@ def test_changelogstatus_entry_dir(patches, is_current): dict(new_callable=PropertyMock)), ("AChangelogStatus.project", dict(new_callable=PropertyMock)), - ("AChangelogStatus.version", - dict(new_callable=PropertyMock)), prefix="envoy.code.check.abstract.changelog") - with patched as (m_current, m_project, m_version): + with patched as (m_current, m_project): m_current.return_value = is_current result = status.entry_dir if not is_current: assert result is None assert not m_project.called - assert not m_version.called return assert ( result - == (m_project.return_value.changelogs - .changelog_path.return_value - .with_suffix.return_value)) - assert ( - m_project.return_value.changelogs.changelog_path.call_args - == [(m_version.return_value, ), {}]) - assert ( - (m_project.return_value.changelogs - .changelog_path.return_value - .with_suffix.call_args) - == [("", ), {}]) + == m_project.return_value.changelogs.current_dir_path) assert "entry_dir" not in status.__dict__