Refactor xlsx gen#59
Conversation
tzschmidt
commented
Mar 31, 2026
- Refactor xlsx_gen
- Move charts from ipynb to spreadsheets -> ipynb_gen removed
- Several fixes regarding spreadsheets
There was a problem hiding this comment.
Pull request overview
Refactors XLSX generation into a dedicated benchmarktool.result.xlsx_gen package with separate sheet classes (instance/merged/class + helper/chart sheets), removes the Jupyter notebook generator path, and updates CLI/docs/tests accordingly.
Changes:
- Replaced the monolithic
result/xlsx_gen.pywith a structuredresult/xlsx_gen/package (core types + result/plot sheet implementations). - Removed
ipynb_genand the--jupyter-notebookCLI option; updated docs and dependencies to match. - Split/rewrote XLSX-related tests into targeted modules under
tests/result/xlsx_gen/.
Reviewed changes
Copilot reviewed 27 out of 31 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_entry.py | Updates CLI conversion tests to reflect removal of the Jupyter notebook option. |
| tests/result/xlsx_gen/test_xlsx_gen.py | Adds unit tests for new spreadsheet primitives (Formula, DataValidation, Chart, XLSXDoc, etc.). |
| tests/result/xlsx_gen/test_result_sheet.py | Adds focused tests for shared ResultSheet behaviors (summaries, styles, write logic). |
| tests/result/xlsx_gen/test_merge_sheet.py | Adds tests for merged-runs sheet logic and summary generation behavior. |
| tests/result/xlsx_gen/test_instance_sheet.py | Adds tests for per-instance sheet layout and summary logic. |
| tests/result/xlsx_gen/test_helper_sheet.py | Adds tests for helper-sheet table construction used by charting. |
| tests/result/xlsx_gen/test_class_sheet.py | Adds tests for class-aggregation sheet behavior. |
| tests/result/xlsx_gen/test_chart_sheet.py | Adds tests for chart-sheet setup and chart object insertion. |
| tests/result/xlsx_gen/init.py | Introduces the test package namespace for XLSX generator tests. |
| tests/result/test_xlsx_gen.py | Removes legacy monolithic XLSX generator tests (superseded by granular tests). |
| tests/result/test_result_parser.py | Updates expectations for parsed measures based on updated reference XML. |
| tests/result/test_result_classes.py | Updates spreadsheet generation tests for new XLSXDoc API (finalize) and new sheet classes. |
| tests/result/test_ipynb_gen.py | Removes tests for the deleted Jupyter notebook generator. |
| tests/ref/test_eval.xml | Adjusts fixture XML to cover missing/mixed measure scenarios used by new XLSX tests. |
| src/benchmarktool/result/xlsx_gen/xlsx_gen.py | Adds new XLSXDoc orchestrating instance/merged/class/helper/chart sheets. |
| src/benchmarktool/result/xlsx_gen/spreadsheet.py | Adds core spreadsheet primitives (sheet base, formulas, validations, charts, helpers, system blocks). |
| src/benchmarktool/result/xlsx_gen/result_sheets/result_sheet.py | Adds shared result-sheet implementation (finalize/summaries/styles/write). |
| src/benchmarktool/result/xlsx_gen/result_sheets/merged_sheet.py | Adds merged-runs sheet implementation (merge criteria + formulas). |
| src/benchmarktool/result/xlsx_gen/result_sheets/instance_sheet.py | Adds instance-results sheet implementation (runs + per-column summaries). |
| src/benchmarktool/result/xlsx_gen/result_sheets/class_sheet.py | Adds class-summary sheet implementation (aggregation over instances). |
| src/benchmarktool/result/xlsx_gen/result_sheets/init.py | Introduces package namespace for result sheet implementations. |
| src/benchmarktool/result/xlsx_gen/plot_sheets/helper_sheet.py | Adds helper sheet that generates chart-ready tables/formulas. |
| src/benchmarktool/result/xlsx_gen/plot_sheets/chart_sheet.py | Adds chart sheet with user-selectable merge/measure and embedded charts. |
| src/benchmarktool/result/xlsx_gen/plot_sheets/init.py | Introduces package namespace for plot sheet implementations. |
| src/benchmarktool/result/xlsx_gen/init.py | Introduces xlsx_gen package namespace. |
| src/benchmarktool/result/xlsx_gen.py | Removes legacy monolithic XLSX generator implementation. |
| src/benchmarktool/result/result.py | Switches to new XLSXDoc import path and renames finish() to finalize(). |
| src/benchmarktool/result/ipynb_gen.py | Removes Jupyter notebook generator module. |
| src/benchmarktool/entry_points.py | Removes --jupyter-notebook flag and notebook generation logic from CLI. |
| pyproject.toml | Removes nbformat dependency and the plot extra since notebook generation was removed. |
| docs/getting_started/conv/index.md | Updates documentation to describe built-in spreadsheet plots and remove notebook instructions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,201 @@ | |||
| """ | |||
There was a problem hiding this comment.
There is quite some duplication in the code for the different sheets that could probably be reduced by applying some patterns (e.g. template method) / restructuring. Not sure whether this is is worth the effort, though. However, maybe at least consider some low hanging fruits
like e.g. extracting a common enumerate(["SUM", "AVG", "DEV", "DST", "BEST", "BETTER", "WORSE", "WORST"]
Anyhow, given that I'm not a python expert, I'll let @rkaminsk approve this PR 😅
There was a problem hiding this comment.
This is way to big to fully review in reasonable time. I'd say just merge!