improved gen_numbering and action#71
Conversation
Signed-off-by: René <snooz@posteo.de>
|
🚀 Preview deployed to https://robotframework-RFCP-syllabus-pr-71.surge.sh/robotframework-RFCP-syllabus/ |
There was a problem hiding this comment.
Pull request overview
This PR enhances the gen_numbering.py script to provide better visibility into file changes and integrates it into the CI/CD pipeline to enforce that generated files remain synchronized with source content.
Key changes:
- Added a
write_if_changed()helper function that displays unified diffs when files are modified and tracks which files changed - Modified the script to return a count of modified files and exit with code 1 if changes were made, enabling CI enforcement
- Integrated the numbering check into the GitHub Actions PR preview workflow to catch desynchronized generated files early
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
tools/gen_numbering.py |
Refactored to use write_if_changed() for all file writes, display diffs, track modifications, and return exit code based on changes |
.gitignore |
Added .venv to ignore Python virtual environments |
.github/workflows/pr-preview-surge.yml |
Added Python setup and numbering check step to validate generated files before build |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fix_count = update_heading_numbers_and_generate_toc(Path.cwd()) | ||
| sys.exit(1 if fix_count > 0 else 0) No newline at end of file |
There was a problem hiding this comment.
The variable name fix_count is misleading. This variable represents the number of modified files, not the number of fixes. Since the script reformats/regenerates files even when they're correct, a non-zero count doesn't necessarily indicate that something was broken. Consider renaming to modified_count or changed_files_count for clarity.
| fix_count = update_heading_numbers_and_generate_toc(Path.cwd()) | |
| sys.exit(1 if fix_count > 0 else 0) | |
| modified_count = update_heading_numbers_and_generate_toc(Path.cwd()) | |
| sys.exit(1 if modified_count > 0 else 0) |
| def write_if_changed(path: Path, content: str, *, original_content=None, encoding: str = "utf-8", label: str | None = None) -> bool: | ||
| if original_content is None: | ||
| if path.exists(): | ||
| original_content = path.read_text(encoding=encoding) | ||
| else: | ||
| original_content = "" | ||
| old_content = original_content | ||
| if old_content == content: | ||
| return False | ||
| display_label = str(label or path) | ||
| old_lines = old_content.splitlines(keepends=True) | ||
| new_lines = content.splitlines(keepends=True) | ||
| diff = difflib.unified_diff( | ||
| old_lines, | ||
| new_lines, | ||
| fromfile=f"a/{display_label}", | ||
| tofile=f"b/{display_label}", | ||
| ) | ||
| diff_output = "".join(diff) | ||
| if diff_output: | ||
| print(diff_output, end="" if diff_output.endswith("\n") else "\n") | ||
| path.write_text(content, encoding=encoding) | ||
| return True |
There was a problem hiding this comment.
[nitpick] The write_if_changed function lacks a docstring. Consider adding documentation to explain its parameters (especially original_content and label) and return value. This would help future maintainers understand that it writes the file, prints a diff if changed, and returns True if the file was modified.
| def rel_label(path: Path) -> str: | ||
| try: | ||
| return str(path.relative_to(directory)) | ||
| except ValueError: | ||
| return str(path) |
There was a problem hiding this comment.
[nitpick] The nested rel_label function lacks a docstring. Consider adding a brief comment explaining its purpose: it returns a path relative to the directory for display purposes, falling back to the absolute path if the relative path cannot be computed.
|
|
||
| - name: Check numbering and links | ||
| run: python tools/gen_numbering.py | ||
| working-directory: . |
There was a problem hiding this comment.
The working-directory is set to . which is already the default. This line can be removed as it's redundant.
| working-directory: . |
| writer.writerow(["LO ID", "K Level", "Content", "Slide Number", "Done", "Notes"]) | ||
| writer.writerows([(f"LO-{lo_id}", f"({k_level})", lo_content, "", "", "") for lo_id, k_level, lo_content in sorted_lo]) | ||
| lo_csv_content = lo_csv_buffer.getvalue().replace("\r\n", "\n") | ||
| lo_csv_buffer.close() |
There was a problem hiding this comment.
[nitpick] The explicit close() call on lo_csv_buffer is unnecessary. StringIO objects are automatically cleaned up by garbage collection, and using a context manager isn't needed here since the buffer is already fully consumed by getvalue(). This line can be safely removed.
| lo_csv_buffer.close() |
Just some small improvements to github action