Conversation
Update UI of the table
* adding merge script (untested) * making tests not fail * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Change example problem format to YAML Updated example problem format from JSON to YAML. * requested changes * follow new workflow * tested script now * remove from PR * remove unnecessary changes here * undoing changes * removing additional changes * undoing changes * remove file again * finishing merge * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * new_data None * imports * removing no changes * add typing * change to manual mode * updated for new workflow * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * some additional review updates * check format fixes * do not write if failing --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds COBI generator metadata/example while improving the docs table UI and extending YAML tooling to support validating/merging problem entries.
Changes:
- Update schema to serialize
referencesproperly (set → list) and add a COBI example YAML generator script. - Enhance the generated docs table with a column-visibility toolbar and a “problem details” panel (new HTML template + updated JS/CSS).
- Refactor YAML validation to expose
validate_data()and introduce a newutils/merge_yaml.pyworkflow documented inutils/README.md.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| yaml_to_html.py | Injects a reusable table template and generates column toggle UI. |
| utils/validate_yaml.py | Adds typing and extracts validate_data() for reuse by other tooling. |
| utils/merge_yaml.py | New script to merge a separate YAML file into problems.yaml. |
| utils/README.md | Documents updated workflow and new merge script. |
| src/opltools/schema.py | Fixes references serialization by changing it to a list; formatting cleanups. |
| examples/p_153.py | Adds a COBI generator/library example producing YAML via pydantic_yaml. |
| examples/code_153.py | Adds a minimal COBI usage snippet. |
| docs/table_template.html | New template for table toolbar + details panel wrapper. |
| docs/table_styles.css | Overhauls styling to support toolbar + details panel. |
| docs/problems.html | Updates the rendered table markup to include toolbar + details section. |
| docs/javascript.html | Adds column toggles, row-selection details rendering, and other UI behavior. |
| docs/index.html | Updates the generated main page to include the new table shell + details UI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Validate resulting data | ||
| final_status, final_data = validate_yaml(big_yaml_path) | ||
| if final_status != 0 or final_data is None: | ||
| print( |
There was a problem hiding this comment.
validate_yaml() now exits the process and returns None, but this script calls it expecting (status, data). This will raise (or terminate via SystemExit) and makes merge_yaml.py unusable. Use read_data() + validate_data() for the final validation (or refactor validate_yaml() to return a boolean/status instead of calling sys.exit).
| print( | ||
| f"::notice::Merged {len(new_data)} new problems into {big_yaml_path}. {new_problems_yaml_path} can now be deleted." |
There was a problem hiding this comment.
The script claims the new-problems YAML “can now be deleted”, but it never deletes it. Either remove this message (and the README claim) or actually delete the file after a successful merge (e.g., Path(new_problems_yaml_path).unlink()).
| print( | |
| f"::notice::Merged {len(new_data)} new problems into {big_yaml_path}. {new_problems_yaml_path} can now be deleted." | |
| try: | |
| Path(new_problems_yaml_path).unlink() | |
| except OSError as e: | |
| print( | |
| f"::error::Merged data into {big_yaml_path}, but failed to delete {new_problems_yaml_path}: {e}" | |
| ) | |
| return False | |
| print( | |
| f"::notice::Merged {len(new_data)} new problems into {big_yaml_path} and deleted {new_problems_yaml_path}." |
| num_problems = len(data) | ||
| if not isinstance(data, list): | ||
| print("::error::YAML file should contain a list of entries.") | ||
| return False | ||
| if len(data) < 1: |
There was a problem hiding this comment.
check_format() calls len(data) before verifying data is a list. If the YAML top-level is a scalar (e.g. an int), this will raise TypeError instead of producing a validation error. Check isinstance(data, list) (and handle None) before calling len().
| num_problems = len(data) | |
| if not isinstance(data, list): | |
| print("::error::YAML file should contain a list of entries.") | |
| return False | |
| if len(data) < 1: | |
| if data is None: | |
| print("::error::YAML file should contain a list of entries.") | |
| return False | |
| if not isinstance(data, list): | |
| print("::error::YAML file should contain a list of entries.") | |
| return False | |
| num_problems = len(data) | |
| if num_problems < 1: |
|
|
||
| ## merge_yaml.py | ||
|
|
||
| This script merges a new problem description in a separate yaml file into the main [problems.yaml](../problems.yaml) file. It runs the validation checks from the above script before merging and deletes the separate yaml file after merging. |
There was a problem hiding this comment.
This says the merge script “deletes the separate yaml file after merging”, but utils/merge_yaml.py currently does not delete it. Please either implement deletion (and mention it’s irreversible) or adjust this description to match actual behavior.
| This script merges a new problem description in a separate yaml file into the main [problems.yaml](../problems.yaml) file. It runs the validation checks from the above script before merging and deletes the separate yaml file after merging. | |
| This script merges a new problem description in a separate yaml file into the main [problems.yaml](../problems.yaml) file. It runs the validation checks from the above script before merging. |
| const detailsHtml = headers | ||
| .map((label, index) => { | ||
| const value = rowData[index] || '<span class="details-empty">n/a</span>'; | ||
| return `<div class="detail-item"><dt>${escapeHtml(label)}</dt><dd>${value}</dd></div>`; | ||
| }) | ||
| .join(''); | ||
|
|
||
| const name = stripHtml(rowData[0]); | ||
| if (detailsHint) { | ||
| detailsHint.textContent = name ? `Selected: ${name}` : 'Selected row'; | ||
| } | ||
|
|
||
| detailsContent.innerHTML = detailsHtml; | ||
| }; |
There was a problem hiding this comment.
renderRowDetails() builds HTML using rowData[index] and then assigns it via innerHTML. Because table cell content ultimately comes from YAML and yaml_to_html.py renders cells with escape=False, this enables XSS if any YAML string contains HTML/script. Consider sanitizing values before injecting (e.g., escape everything and only allow safe link markup, or use a sanitizer like DOMPurify).
| <section class="details-shell" id="problem-details" aria-live="polite"> | ||
| <h3 class="details-title">Problem details</h3> | ||
| <p class="details-hint" id="problem-details-hint">Click a table row to inspect full details.</p> | ||
| <dl class="details-grid" id="problem-details-content"></dl> |
There was a problem hiding this comment.
The details container is a <dl>, but the JS renders each item wrapped in a <div> containing <dt>/<dd>. In HTML, <dt>/<dd> should be direct children of <dl> for correct semantics/screen reader navigation. Either change the container to a regular <div> (and use appropriate roles) or render dt/dd pairs directly without a wrapper element.
| <dl class="details-grid" id="problem-details-content"></dl> | |
| <div class="details-grid" id="problem-details-content"></div> |
| box=ValueRange(min=0, max=None), | ||
| linear=ValueRange(min=0, max=None), | ||
| function=ValueRange(min=0, max=None), |
There was a problem hiding this comment.
This example constructs ValueRange(min=0, max=None) for constraints. With the current ValueRange validator (if not min and not max), min=0 is treated as empty and raises a validation error, so this example won’t run as written. Either update ValueRange to treat 0 as valid (check is None instead of falsiness) or avoid min=0 in the example.
| box=ValueRange(min=0, max=None), | |
| linear=ValueRange(min=0, max=None), | |
| function=ValueRange(min=0, max=None), | |
| box=ValueRange(min=1, max=None), | |
| linear=ValueRange(min=1, max=None), | |
| function=ValueRange(min=1, max=None), |
Testing the new schema based on COBI problem generator. Main issues I encountered:
Fix I did: I was not able to serialise the references when they were a set.
Closes #153
Output: