support yaml as format for patterns#71
Conversation
|
Note that the collectors-teeing pattern was rewritten to yaml, for testing purposes. You may want to keep the original, even though IMO the yaml version is easier to read. I know you are known to dislike and bash yaml, and in general I agree, but in this case ... |
There was a problem hiding this comment.
Pull request overview
Adds YAML (.yaml/.yml) as an alternative source format for pattern definitions so multi-line code snippets are easier to author, updating both the Python and Java HTML generators and migrating one existing pattern from JSON to YAML.
Changes:
- Update
html-generators/generate.pyto load patterns from JSON and YAML files. - Update
html-generators/generate.javato parse JSON and YAML via Jackson (YAMLFactory). - Convert
content/collections/collectors-teeingpattern from.jsonto.yaml.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| html-generators/generate.py | Adds YAML parsing support to the Python generator when loading snippet files. |
| html-generators/generate.java | Adds Jackson YAML dependency and chooses mapper based on file extension. |
| content/collections/collectors-teeing.yaml | New YAML version of the collectors-teeing pattern content. |
| content/collections/collectors-teeing.json | Removes the JSON version of the collectors-teeing pattern. |
Comments suppressed due to low confidence (2)
html-generators/generate.java:14
- The class Javadoc still says it generates pages from JSON snippet files, but the implementation now supports YAML too. Update the Javadoc to reflect the supported formats so it stays accurate.
This issue also appears on line 139 of the same file.
/**
* Generate HTML detail pages from JSON snippet files and slug-template.html.
* JBang equivalent of generate.py — produces identical output.
*/
html-generators/generate.java:140
- Typo in comment: “sortall” -> “sort all”.
// first collect and sortall files
for (var ext : MAPPERS.keySet()) {
| oldCode: | | ||
| long count = items.stream().count(); | ||
| double sum = items.stream() | ||
| .mapToDouble(Item::price) | ||
| .sum(); | ||
| var result = new Stats(count, sum); | ||
| modernCode: | | ||
| var result = items.stream().collect( | ||
| Collectors.teeing( | ||
| Collectors.counting(), | ||
| Collectors.summingDouble(Item::price), | ||
| Stats::new | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Using YAML block scalars with | will include a trailing newline in the resulting oldCode/modernCode strings, which can make generated output differ from the previous JSON version. If you want byte-for-byte stability, use |- (strip final newline) for code blocks or strip a single trailing newline in the generators when reading YAML.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| """Generate HTML detail pages from JSON snippet files and slug-template.html.""" | ||
|
|
||
| import json | ||
| import yaml | ||
| import glob |
There was a problem hiding this comment.
import yaml adds a new runtime dependency (PyYAML). As-is, running python3 html-generators/generate.py will fail with ImportError on machines that don’t already have it. Consider importing YAML conditionally with a clear error message (e.g., instruct to install PyYAML), and update the module docstring to reflect JSON+YAML support.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| json_files = [] | ||
| for cat in CATEGORY_DISPLAY: | ||
| json_files.extend(sorted(glob.glob(f"{CONTENT_DIR}/{cat}/*.json"))) | ||
| json_files.extend(glob.glob(f"{CONTENT_DIR}/{cat}/*.json")) | ||
| json_files.extend(glob.glob(f"{CONTENT_DIR}/{cat}/*.yaml")) | ||
| json_files.extend(glob.glob(f"{CONTENT_DIR}/{cat}/*.yml")) | ||
| json_files.sort() |
There was a problem hiding this comment.
The new json_files.sort() globally sorts all snippet paths across categories, which changes the insertion order of all_snippets (and therefore the order of cards in index.html and entries in site/data/snippets.json). Previously ordering followed categories.properties category order. Also, this makes Python output diverge from generate.java, which still preserves category order. Preserve per-category ordering (sort within each category) or update both generators to use the same deterministic ordering strategy.
See below for a potential fix:
# Preserve category order from CATEGORY_DISPLAY and sort files within each category
for cat in CATEGORY_DISPLAY:
cat_files = []
cat_files.extend(glob.glob(f"{CONTENT_DIR}/{cat}/*.json"))
cat_files.extend(glob.glob(f"{CONTENT_DIR}/{cat}/*.yaml"))
cat_files.extend(glob.glob(f"{CONTENT_DIR}/{cat}/*.yml"))
for path in sorted(cat_files):
json_files.append(path)
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| with open(path) as f: | ||
| data = json.load(f) | ||
| if path.endswith(".yaml") or path.endswith(".yml"): | ||
| data = yaml.safe_load(f) | ||
| else: | ||
| data = json.load(f) | ||
| key = f"{data['category']}/{data['slug']}" |
There was a problem hiding this comment.
YAML parsing can produce non-string scalars (e.g., jdkVersion: 12 becomes an int). replace_tokens() expects all replacement values to be strings; returning an int from the replacer will raise a TypeError in re.sub. To make YAML support robust, coerce token values to strings (e.g., in replace_tokens() or normalize fields like jdkVersion right after safe_load).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| ///usr/bin/env jbang "$0" "$@" ; exit $? | ||
| //JAVA 25 | ||
| //DEPS com.fasterxml.jackson.core:jackson-databind:2.18.3 | ||
| //DEPS com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:2.18.3 | ||
|
|
There was a problem hiding this comment.
PR adds YAML support for patterns, but the Pages deploy workflow currently only triggers on content/**/*.json. That means future changes to .yaml/.yml patterns won’t automatically redeploy the site. Update the workflow path filters to include YAML extensions so the feature works end-to-end in CI.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
2 participants
fixes issue #70 for both generators