refactor: simplify CSVWriter into a single primary function for appending rows - #357
Conversation
315f2fc to
187b930
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors CSVWriter to remove unused functionality, simplify row-buffering by making add_rows variadic, and adds unit tests to validate CSV output behavior.
Changes:
- Simplifies
CSVWriterby removing unused methods and makingadd_rows(*rows)variadic. - Updates call sites to use the new
add_rowsshape (including*audit_resultwhere appropriate). - Adds a dedicated
tests/test_output.pysuite covering CSV write behavior (no rows, header, BOM, multiple writes, column ordering).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_output.py | Adds tests validating CSVWriter behavior for headers/BOM/multiple writes/column ordering. |
| src/output.py | Refactors CSVWriter API (variadic add_rows, simplified file writing, internal lock helper). |
| src/crawler.py | Updates CSV writing call sites to use the new add_rows API. |
| src/audit_manager.py | Updates results writing to splat list results into add_rows, and adds a (currently flawed) type annotation. |
Suppressed comments (1)
src/output.py:52
- The
add_rowsdocstring still describes a singlelist[...]argument, but the method is now variadic (*rows). This makes the doc misleading for callers and for generated docs.
def add_rows(self, *rows: dict[Any, Any]) -> None:
"""Add a list of rows to the CSV row buffer.
Args:
rows (list[dict[Any, Any]]): list of rows of data
"""
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| file_already_exists = os.path.exists(path) | ||
| with open(path, 'a', encoding='utf-8-sig') as csvfile: | ||
| writer = csv.DictWriter(csvfile, fieldnames=keys) | ||
| if not file_exists: | ||
| if not file_already_exists: | ||
| writer.writeheader() |
There was a problem hiding this comment.
this isn't new, though I might address it as part of dealing with #348 (comment)
CSVWriterCSVWriter into a single primary function for appending rows
| def append_rows(self, path: str, *rows: dict[Any, Any]) -> None: | ||
| """Add a list of rows to a CSV file. | ||
|
|
||
| Args: | ||
| path (str): path to file |
There was a problem hiding this comment.
@copilot what's the right way to document variadic params in Python docstrings?
There was a problem hiding this comment.
Use the starred name in the docstring arg list, e.g. *rows (dict[str, Any]): one or more rows of data (or “variable number of row dicts”). I’ve updated append_rows to that form and tightened the type hints in commit 7fb39b5.
There was a problem hiding this comment.
I asked you a question, not to do an action 🤦
7fb39b5 to
07f9adf
Compare
When reviewing this class as part of #348, I realized there were some unused and duplicated code, but then later realized ultimately what we're using this for is locking the file to do thread safe writes, and appending rows to a csv file.
In light of that, I've ended up refactoring the class to a single public
append_rowsfunction, along with adding tests