Address PR feedback

Author: jonathan343

scripts/changelog/new-entry.py

@@ -7,36 +7,35 @@
 
 import argparse
 import json
-import time
 import uuid
 from pathlib import Path
 
+from utils import get_package_changes_dir
+
 PROJECT_ROOT_DIR = Path(__file__).resolve().parent.parent.parent
 
 
-def get_package_changes_dir(package_name: str) -> Path:
-    package_path = PROJECT_ROOT_DIR / "clients" / package_name
-    changes_dir = package_path / ".changes"
-    return changes_dir
+def setup_changes_directories(package_name: str) -> Path:
+    """Set up and return the next-release directory for a package."""
+    changes_dir = get_package_changes_dir(package_name)
+    changes_dir.mkdir(exist_ok=True)
+
+    next_release_dir = changes_dir / "next-release"
+    next_release_dir.mkdir(exist_ok=True)
+
+    return next_release_dir
 
 
 def create_change_entry(
     change_type: str,
     description: str,
     package_name: str,
 ) -> str:
-    # Get package .changes directory and ensure it exists
-    changes_dir = get_package_changes_dir(package_name)
-    changes_dir.mkdir(exist_ok=True)
-
-    # Create next-release directory for pending changes
-    next_release_dir = changes_dir / "next-release"
-    next_release_dir.mkdir(exist_ok=True)
+    next_release_dir = setup_changes_directories(package_name)
 
     # Generate unique filename
-    timestamp = int(time.time() * 1_000_000)
     unique_id = uuid.uuid4().hex
-    filename = f"{package_name}-{change_type}-{timestamp}-{unique_id}.json"
+    filename = f"{package_name}-{change_type}-{unique_id}.json"
 
     entry_data = {
         "type": change_type,
@@ -56,13 +55,7 @@ def create_summary_entry(
     package_name: str,
 ) -> str:
     """Create or update the release summary for the next release."""
-    # Get package .changes directory and ensure it exists
-    changes_dir = get_package_changes_dir(package_name)
-    changes_dir.mkdir(exist_ok=True)
-
-    # Create next-release directory for pending changes
-    next_release_dir = changes_dir / "next-release"
-    next_release_dir.mkdir(exist_ok=True)
+    next_release_dir = setup_changes_directories(package_name)
 
     # Summary is stored in a fixed file (only one summary per release)
     summary_file = next_release_dir / "SUMMARY.json"
@@ -86,7 +79,7 @@ def main():
     parser.add_argument(
         "-t",
         "--type",
-        # TODO: Remove the 'breaking' option once this project is stable.
+        # TODO: Prompt the user for confirmation before allowing the 'breaking' or `feature` options.
         choices=(
             "feature",
             "enhancement",

scripts/changelog/new-release.py

@@ -13,6 +13,8 @@
 from pathlib import Path
 from typing import Any
 
+from utils import get_package_changes_dir
+
 PROJECT_ROOT_DIR = Path(__file__).resolve().parent.parent.parent
 VERSION_PATTERN = r"^\d+\.\d+\.\d+$"
 CHANGE_TYPES_ORDER = {
@@ -30,12 +32,16 @@ def validate_change_entry(change_data: dict[str, Any], entry_file: Path) -> bool
     if "type" not in change_data or change_data["type"] not in CHANGE_TYPES:
         print(
             f"Error: Missing or invalid 'type' field in {entry_file}\n"
-            f"Type must be one of: {CHANGE_TYPES}"
+            f"Type must be one of: {CHANGE_TYPES}",
+            file=sys.stderr,
         )
         return False
 
     if "description" not in change_data or not change_data["description"]:
-        print(f"Error: Missing or empty 'description' field in {entry_file}")
+        print(
+            f"Error: Missing or empty 'description' field in {entry_file}",
+            file=sys.stderr,
+        )
         return False
 
     return True
@@ -53,7 +59,9 @@ def collect_next_release_summary(next_release_dir: Path) -> str | None:
             data = json.load(f)
             return data.get("summary")
     except (OSError, json.JSONDecodeError) as e:
-        print(f"Warning: Could not read summary file {summary_file}: {e}", file=sys.stderr)
+        print(
+            f"Warning: Could not read summary file {summary_file}: {e}", file=sys.stderr
+        )
         return None
 
 
@@ -97,7 +105,7 @@ def create_version_file(
     version_file = changes_dir / f"{version}.json"
 
     if version_file.exists():
-        print(f"Error: Version file {version_file} already exists!")
+        print(f"Error: Version file {version_file} already exists!", file=sys.stderr)
         sys.exit(1)
 
     version_data: dict[str, Any] = {"changes": changes}
@@ -107,6 +115,7 @@ def create_version_file(
 
     with open(version_file, "w") as f:
         json.dump(version_data, f, indent=2)
+        f.write("\n")
 
     return version_file
 
@@ -116,22 +125,22 @@ def cleanup_next_release_dir(next_release_dir: Path) -> int:
 
     for entry_file in next_release_dir.iterdir():
         if entry_file.is_file() and entry_file.suffix == ".json":
-            try:
-                entry_file.unlink()
-                removed_count += 1
-            except OSError as e:
-                print(f"Warning: Could not remove {entry_file}: {e}", file=sys.stderr)
+            entry_file.unlink()
+            removed_count += 1
 
     return removed_count
 
 
 def create_new_release(package_name: str, version: str, dry_run: bool = False) -> int:
-    # Get package directories
-    changes_dir = PROJECT_ROOT_DIR / "clients" / package_name / ".changes"
+    # Get package directories (validates package exists)
+    changes_dir = get_package_changes_dir(package_name)
     next_release_dir = changes_dir / "next-release"
 
     if not changes_dir.exists():
-        print(f"Error: No .changes directory found for package: {package_name}")
+        print(
+            f"Error: No .changes directory found for package: {package_name}",
+            file=sys.stderr,
+        )
         return 1
 
     # Collect summary and changes from next-release
@@ -141,7 +150,8 @@ def create_new_release(package_name: str, version: str, dry_run: bool = False) -
     if not changes:
         print(
             f"No changelog entries found in {next_release_dir}.\n"
-            "Use 'python scripts/changelog/new-entry.py' to create entries first"
+            "Use 'python scripts/changelog/new-entry.py' to create entries first",
+            file=sys.stderr,
         )
         return 1
 
@@ -164,7 +174,7 @@ def create_new_release(package_name: str, version: str, dry_run: bool = False) -
         version_file = create_version_file(changes_dir, version, changes, summary)
         print(f"\nCreated version file: {version_file}")
     except Exception as e:
-        print(f"Error creating version file: {e}")
+        print(f"Error creating version file: {e}", file=sys.stderr)
         return 1
 
     # Clean up next-release directory
@@ -193,7 +203,7 @@ def main() -> int:
 
     # Basic version format validation
     if not bool(re.match(VERSION_PATTERN, args.version)):
-        print("Error: Version must be in format x.y.z (e.g., 1.2.3)")
+        print("Error: Version must be in format x.y.z (e.g., 1.2.3)", file=sys.stderr)
         return 1
 
     return create_new_release(

scripts/changelog/render.py

@@ -13,6 +13,7 @@
 from typing import IO, Any
 
 from jinja2 import Template
+from utils import get_package_changes_dir
 
 PROJECT_ROOT_DIR = Path(__file__).resolve().parent.parent.parent
 
@@ -21,7 +22,7 @@
 VERSION_FILE_PATTERN = r"^\d+\.\d+\.\d+\.json$"
 
 
-def get_sorted_versions(changes_dir: Path) -> list[str]:
+def get_sorted_versions(changes_dir: Path, reverse: bool = True) -> list[str]:
     """Get sorted list of version numbers from .changes directory."""
     version_pattern = re.compile(VERSION_FILE_PATTERN)
     versions: list[str] = []
@@ -30,16 +31,16 @@ def get_sorted_versions(changes_dir: Path) -> list[str]:
         if file.is_file() and version_pattern.match(file.name):
             versions.append(file.stem)
 
-    # Sort by semantic version (oldest first)
-    versions.sort(key=lambda v: [int(x) for x in v.split(".")])
+    # Sort by semantic version (reverse if True)
+    versions.sort(key=lambda v: [int(x) for x in v.split(".")], reverse=reverse)
     return versions
 
 
 def load_package_releases(changes_dir: Path) -> dict[str, dict[str, Any]]:
     """Load all changelog entries from version JSON files."""
     releases: dict[str, dict[str, Any]] = {}
 
-    for version_number in get_sorted_versions(changes_dir):
+    for version_number in get_sorted_versions(changes_dir, reverse=True):
         filename = changes_dir / f"{version_number}.json"
         try:
             with open(filename) as f:
@@ -75,7 +76,7 @@ def render_changes(
 ) -> None:
     """Render changelog using Jinja template."""
     # Reverse order to show newest first
-    context: dict[str, Any] = {"releases": reversed(list(changes.items()))}
+    context: dict[str, Any] = {"releases": list(changes.items())}
 
     template = Template(template_contents)
 
@@ -86,33 +87,34 @@ def render_changes(
 def render_package_changelog(
     package_name: str, template_name: str | None = None, output_path: Path | None = None
 ) -> int:
-    # Determine changes directory from package name
-    package_dir = PROJECT_ROOT_DIR / "clients" / package_name
-    changes_dir = package_dir / ".changes"
+    # Get changes directory (validates package exists)
+    changes_dir = get_package_changes_dir(package_name)
 
     if not changes_dir.exists():
-        print(f"No .changes directory found for package: {package_name}")
+        print(
+            f"No .changes directory found for package: {package_name}", file=sys.stderr
+        )
         return 1
 
     # Load changes from the directory
     changes: dict[str, dict[str, Any]] = load_package_releases(changes_dir)
 
     if not changes:
-        print(f"No version JSON files found in {changes_dir}")
+        print(f"No version JSON files found in {changes_dir}", file=sys.stderr)
         return 1
 
     # Get template contents
     template_path = TEMPLATES_DIR / (template_name or DEFAULT_TEMPLATE_NAME)
 
     if not template_path.exists():
-        print(f"Template not found: {template_path}")
+        print(f"Template not found: {template_path}", file=sys.stderr)
         return 1
 
     try:
         with open(template_path) as f:
             template_contents = f.read()
     except OSError as e:
-        print(f"Error reading template {template_path}: {e}")
+        print(f"Error reading template {template_path}: {e}", file=sys.stderr)
         return 1
 
     # Render to output

scripts/changelog/utils.py

@@ -0,0 +1,28 @@
+#!/usr/bin/env python3
+# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+# SPDX-License-Identifier: Apache-2.0
+"""
+Common utilities for changelog management scripts.
+"""
+
+import sys
+from pathlib import Path
+
+PROJECT_ROOT_DIR = Path(__file__).resolve().parent.parent.parent
+
+
+def validate_package_name(package_name: str) -> None:
+    """Validate that the package exists in the clients directory."""
+    package_path = PROJECT_ROOT_DIR / "clients" / package_name
+    if not package_path.exists():
+        print(
+            f"Error: Package '{package_name}' not found in clients directory",
+            file=sys.stderr,
+        )
+        sys.exit(1)
+
+
+def get_package_changes_dir(package_name: str) -> Path:
+    """Get the .changes directory for a package."""
+    validate_package_name(package_name)
+    return PROJECT_ROOT_DIR / "clients" / package_name / ".changes"