From 7da31db53c453dfcd14e6081b61d7fd38e9ff497 Mon Sep 17 00:00:00 2001 From: Jing Chen Date: Tue, 23 Jun 2026 13:58:30 -0400 Subject: [PATCH 1/2] Add example broken lesson as a teaching sample for the PR check Single lesson YAML with several intentional errors (missing required field, list-as-string for authors/platforms/tags, material with no URL, multi-value recommended_platform) to demonstrate how CI validation responds to a bad submission. Surfaced one at a time (fail-fast) as students fix and re-push. Co-Authored-By: Claude Opus 4.8 --- lessons/example-broken-lesson.yml | 62 +++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 lessons/example-broken-lesson.yml diff --git a/lessons/example-broken-lesson.yml b/lessons/example-broken-lesson.yml new file mode 100644 index 0000000..505c3a1 --- /dev/null +++ b/lessons/example-broken-lesson.yml @@ -0,0 +1,62 @@ +# ============================================================================= +# EXAMPLE BROKEN LESSON — TEACHING SAMPLE (do not use as a real lesson) +# ============================================================================= +# This file intentionally contains SEVERAL different mistakes so students can +# see how the portal's CI check reacts to a bad submission. +# +# IMPORTANT: the CI reports ONE error per file per run (it stops at the first +# problem it finds). So the workflow is iterative and realistic: +# 1. Open the PR -> CI fails and comments with the first error. +# 2. Fix that one line, push again -> CI re-runs and shows the NEXT error. +# 3. Repeat until the check passes. +# +# The planted errors below, in the order the CI will surface them: +# (A) Missing required field -> `description` is commented out. +# (B) authors as a string -> must be a YAML list ("- item"). +# (C) platforms as a string -> must be a YAML list (this one would crash +# the whole build if it weren't caught). +# (D) tags as a string -> must be a YAML list. +# (E) material missing a URL -> each material needs github_url or colab_url. +# (F) recommended_platform -> must be EXACTLY ONE name that matches an +# entry in `platforms` (not a comma list). +# +# Compare this against template.yaml (the correct reference) to see each fix. +# ============================================================================= + +title: "Example Broken Lesson (teaching sample)" + +# (A) REQUIRED FIELD MISSING — uncomment to fix: +# description: "Brief description for search results." + +programming_skill: "None" +primary_course: "Foundational Module" + +# (B) WRONG: authors must be a list, not a string. +authors: "Dr. Ada Lovelace" + +format: "Single Notebook" + +scientific_objectives: + - "Recognize common YAML submission mistakes" +cyberinfrastructure_objectives: + - "Read and act on a CI error message" + +# (C) WRONG: platforms must be a list, not a string. +platforms: "Google Colab" + +# (D) WRONG: tags must be a list, not a string. +tags: "python" + +# (F) WRONG: recommended_platform must be exactly ONE value that matches an +# entry in `platforms`. A comma-joined string matches nothing. +recommended_platform: "Google Colab, ChemCompute" + +public_repo_url: "https://github.com/act-cms/example-repo" + +materials: + - title: "Part 1" + description: "A material with no link." + type: "notebook" + duration: "1 hour" + # (E) WRONG: a material must have at least one of github_url / colab_url. + # github_url: "https://github.com/act-cms/example-repo/blob/main/01.ipynb" From b54da8b43e40de3485feff0a8d352a389a586d80 Mon Sep 17 00:00:00 2001 From: Jing Chen Date: Tue, 23 Jun 2026 14:03:02 -0400 Subject: [PATCH 2/2] Make lesson validation report all errors at once validate_lesson_data now collects every problem in a lesson and prints each as its own Error: line instead of returning on the first one, so the PR-check comment lists them together. Later checks are guarded against missing/malformed fields; recommended_platform and platform-partial checks are skipped when platforms isn't a valid list to avoid cascading errors. Co-Authored-By: Claude Opus 4.8 --- lessons/example-broken-lesson.yml | 13 +-- scripts/build_site.py | 166 ++++++++++++++++-------------- 2 files changed, 93 insertions(+), 86 deletions(-) diff --git a/lessons/example-broken-lesson.yml b/lessons/example-broken-lesson.yml index 505c3a1..f1f287c 100644 --- a/lessons/example-broken-lesson.yml +++ b/lessons/example-broken-lesson.yml @@ -4,13 +4,10 @@ # This file intentionally contains SEVERAL different mistakes so students can # see how the portal's CI check reacts to a bad submission. # -# IMPORTANT: the CI reports ONE error per file per run (it stops at the first -# problem it finds). So the workflow is iterative and realistic: -# 1. Open the PR -> CI fails and comments with the first error. -# 2. Fix that one line, push again -> CI re-runs and shows the NEXT error. -# 3. Repeat until the check passes. +# The CI lists ALL independent errors it can find in one comment, so you can +# fix several at once and push again until the check passes. # -# The planted errors below, in the order the CI will surface them: +# The planted errors: # (A) Missing required field -> `description` is commented out. # (B) authors as a string -> must be a YAML list ("- item"). # (C) platforms as a string -> must be a YAML list (this one would crash @@ -20,6 +17,10 @@ # (F) recommended_platform -> must be EXACTLY ONE name that matches an # entry in `platforms` (not a comma list). # +# Note: error (F) only appears AFTER you fix `platforms` (C). While `platforms` +# is malformed the CI can't meaningfully check recommended_platform against it, +# so it waits rather than printing a confusing cascading error. +# # Compare this against template.yaml (the correct reference) to see each fix. # ============================================================================= diff --git a/scripts/build_site.py b/scripts/build_site.py index a1affc9..f254040 100644 --- a/scripts/build_site.py +++ b/scripts/build_site.py @@ -351,8 +351,23 @@ def get_platform_key(platform): return platform.lower().replace(' ', '_') +def is_list_of_strings(value): + """True if value is a list whose items are all strings""" + return isinstance(value, list) and all(isinstance(item, str) for item in value) + + def validate_lesson_data(lesson_data, filename, templates_dir=None): - """Validate lesson data has required fields""" + """Validate a lesson, collecting ALL problems (not fail-fast). + + Every issue is appended to a list and printed as its own `Error:` line so + the PR-check comment can show the full set at once. Later checks are guarded + so a missing or wrong-typed field never crashes the validator; checks that + genuinely depend on an earlier field being well-formed (e.g. + recommended_platform needs a valid platforms list) are skipped rather than + reported as confusing cascading errors. Returns True only if no errors. + """ + errors = [] + required_fields = [ 'title', 'description', 'programming_skill', 'primary_course', 'authors', 'format', 'scientific_objectives', @@ -369,39 +384,35 @@ def validate_lesson_data(lesson_data, filename, templates_dir=None): 'prerequisite_modules': list } - missing_fields = [] for field in required_fields: if field not in lesson_data: - missing_fields.append(field) - - if missing_fields: - print(f"Error: {filename} is missing required fields: {missing_fields}") - return False + errors.append(f"{filename} is missing required field: '{field}'") # Validate optional instructor fields if present (None == omitted) for field, expected_type in optional_instructor_fields.items(): if field in lesson_data and lesson_data[field] is not None: value = lesson_data[field] if not isinstance(value, expected_type): - print(f"Error: {filename} field '{field}' should be {expected_type.__name__ if hasattr(expected_type, '__name__') else expected_type}") - return False + type_name = expected_type.__name__ if hasattr(expected_type, '__name__') else expected_type + errors.append(f"{filename} field '{field}' should be {type_name}") # Fields the lesson template iterates over: a string where a list is # expected silently renders garbage (or crashes the build, in the case of - # `platforms`). Require these to be non-empty lists of strings. + # `platforms`). Require these to be non-empty lists of strings. Only checked + # when present (a missing one is already reported above). required_list_fields = [ 'authors', 'scientific_objectives', 'cyberinfrastructure_objectives', 'platforms' ] for field in required_list_fields: + if field not in lesson_data: + continue value = lesson_data[field] if not isinstance(value, list) or len(value) == 0: - print(f"Error: {filename} field '{field}' must be a non-empty list " - f"(got {type(value).__name__}). Use YAML '- ' list items, not a single string.") - return False - if not all(isinstance(item, str) for item in value): - print(f"Error: {filename} field '{field}' must be a list of strings") - return False + errors.append(f"{filename} field '{field}' must be a non-empty list " + f"(got {type(value).__name__}). Use YAML '- ' list items, not a single string.") + elif not all(isinstance(item, str) for item in value): + errors.append(f"{filename} field '{field}' must be a list of strings") # Optional list-of-strings fields: only checked when present and non-null. optional_list_fields = [ @@ -410,86 +421,81 @@ def validate_lesson_data(lesson_data, filename, templates_dir=None): for field in optional_list_fields: if field in lesson_data and lesson_data[field] is not None: value = lesson_data[field] - if not isinstance(value, list) or not all(isinstance(item, str) for item in value): - print(f"Error: {filename} field '{field}' must be a list of strings " - f"(got {type(value).__name__}). Use YAML '- ' list items, not a single string.") - return False + if not is_list_of_strings(value): + errors.append(f"{filename} field '{field}' must be a list of strings " + f"(got {type(value).__name__}). Use YAML '- ' list items, not a single string.") # Validate related_modules if present if 'related_modules' in lesson_data and lesson_data['related_modules'] is not None: - if not all(isinstance(module, str) for module in lesson_data['related_modules']): - print(f"Error: {filename} related_modules should be a list of strings") - return False + if not is_list_of_strings(lesson_data['related_modules']): + errors.append(f"{filename} related_modules should be a list of strings") # Check for legacy formats and reject them if 'notebook' in lesson_data or 'notebooks' in lesson_data: - print(f"Error: {filename} uses legacy notebook/notebooks format. Please convert to materials format with explicit URLs.") - return False - - # Validate materials structure - if not isinstance(lesson_data['materials'], list): - print(f"Error: {filename} materials field must be a list") - return False - - if len(lesson_data['materials']) == 0: - print(f"Error: {filename} materials list cannot be empty") - return False - - for i, material in enumerate(lesson_data['materials']): - if not isinstance(material, dict): - print(f"Error: {filename} material {i+1} must be a mapping with title/description/type/duration") - return False - - required_material_fields = ['title', 'description', 'type', 'duration'] - for field in required_material_fields: - if field not in material: - print(f"Error: {filename} material {i+1} is missing required field: {field}") - return False - - # Require at least one URL (github_url or colab_url) - if 'github_url' not in material and 'colab_url' not in material: - print(f"Error: {filename} material {i+1} must have either github_url or colab_url") - return False - - # Validate URL format if present - for url_field in ['github_url', 'colab_url']: - if url_field in material: - url = material[url_field] - if not isinstance(url, str) or not url.startswith('http'): - print(f"Error: {filename} material {i+1} {url_field} must be a valid HTTP/HTTPS URL") - return False - - # objectives is iterated by the template; a string would render garbage - if 'objectives' in material and material['objectives'] is not None: - objectives = material['objectives'] - if not isinstance(objectives, list) or not all(isinstance(o, str) for o in objectives): - print(f"Error: {filename} material {i+1} 'objectives' must be a list of strings " - f"(got {type(objectives).__name__})") - return False - - # recommended_platform must be exactly one of the listed platforms + errors.append(f"{filename} uses legacy notebook/notebooks format. " + f"Please convert to materials format with explicit URLs.") + + # Validate materials structure (only if present — missing already reported) + materials = lesson_data.get('materials') + if 'materials' in lesson_data: + if not isinstance(materials, list): + errors.append(f"{filename} materials field must be a list") + elif len(materials) == 0: + errors.append(f"{filename} materials list cannot be empty") + else: + required_material_fields = ['title', 'description', 'type', 'duration'] + for i, material in enumerate(materials): + if not isinstance(material, dict): + errors.append(f"{filename} material {i+1} must be a mapping with title/description/type/duration") + continue + + for field in required_material_fields: + if field not in material: + errors.append(f"{filename} material {i+1} is missing required field: {field}") + + # Require at least one URL (github_url or colab_url) + if 'github_url' not in material and 'colab_url' not in material: + errors.append(f"{filename} material {i+1} must have either github_url or colab_url") + + # Validate URL format if present + for url_field in ['github_url', 'colab_url']: + if url_field in material: + url = material[url_field] + if not isinstance(url, str) or not url.startswith('http'): + errors.append(f"{filename} material {i+1} {url_field} must be a valid HTTP/HTTPS URL") + + # objectives is iterated by the template; a string would render garbage + if 'objectives' in material and material['objectives'] is not None: + if not is_list_of_strings(material['objectives']): + errors.append(f"{filename} material {i+1} 'objectives' must be a list of strings " + f"(got {type(material['objectives']).__name__})") + + # recommended_platform must be exactly one of the listed platforms. Only + # meaningful when platforms is a valid list of strings; otherwise the bad + # platforms field is already reported and this would just cascade. + platforms = lesson_data.get('platforms') + platforms_ok = is_list_of_strings(platforms) and len(platforms) > 0 if 'recommended_platform' in lesson_data and lesson_data['recommended_platform'] is not None: recommended = lesson_data['recommended_platform'] if not isinstance(recommended, str): - print(f"Error: {filename} recommended_platform must be a single platform name (string)") - return False - if recommended not in lesson_data['platforms']: - print(f"Error: {filename} recommended_platform '{recommended}' must exactly match one entry " - f"in platforms {lesson_data['platforms']} (pick exactly one)") - return False + errors.append(f"{filename} recommended_platform must be a single platform name (string)") + elif platforms_ok and recommended not in platforms: + errors.append(f"{filename} recommended_platform '{recommended}' must exactly match one entry " + f"in platforms {platforms} (pick exactly one)") # Every platform must have a rendering partial, else the build crashes on it - if templates_dir is not None: + if templates_dir is not None and platforms_ok: platforms_dir = Path(templates_dir) / 'platforms' - for platform in lesson_data['platforms']: + for platform in platforms: partial = platforms_dir / f"{get_platform_key(platform)}.html" if not partial.exists(): supported = sorted(p.stem for p in platforms_dir.glob('*.html')) - print(f"Error: {filename} platform '{platform}' is not supported " - f"(no template {partial.name}). Supported platform keys: {supported}") - return False + errors.append(f"{filename} platform '{platform}' is not supported " + f"(no template {partial.name}). Supported platform keys: {supported}") - return True + for error in errors: + print(f"Error: {error}") + return len(errors) == 0 def main(): """Main build process"""