Add CSV and JSON export utilities for PSLab sensor data #260
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds simple CSV and JSON export utilities for PSLab sensor data, implemented as filesystem-based helper functions with minimal validation and accompanying I/O behavior. Sequence diagram for CSV and JSON export utility interactionssequenceDiagram
actor Caller
participant data_export_module
participant csv_module
participant json_module
participant FileSystem
Caller ->> data_export_module: export_to_csv(data, filename)
alt data is empty
data_export_module -->> Caller: return
else data is not empty
data_export_module ->> FileSystem: open(filename, write, newline empty)
FileSystem -->> data_export_module: file_handle
data_export_module ->> csv_module: DictWriter(file_handle, fieldnames from data[0].keys())
csv_module -->> data_export_module: writer
data_export_module ->> writer: writeheader()
data_export_module ->> writer: writerows(data)
data_export_module ->> FileSystem: close(file_handle)
data_export_module -->> Caller: return
end
Caller ->> data_export_module: export_to_json(data, filename)
data_export_module ->> FileSystem: open(filename, write)
FileSystem -->> data_export_module: file_handle
data_export_module ->> json_module: dump(data, file_handle, indent 4)
json_module -->> data_export_module: complete
data_export_module ->> FileSystem: close(file_handle)
data_export_module -->> Caller: return
Flow diagram for CSV and JSON data export utilitiesflowchart TD
A["Start"] --> B["Caller invokes export_to_csv or export_to_json with data and filename"]
B --> C{"Function selected"}
C --> D["export_to_csv(data, filename)"]
C --> H["export_to_json(data, filename)"]
%% CSV path
D --> E{"Is data empty"}
E --> F["Return without creating file"]
E --> G["Open filename for write with newline empty using csv module"]
G --> I["Create DictWriter with fieldnames from first dict keys"]
I --> J["Write header row"]
J --> K["Write all data rows"]
K --> L["Close file"]
L --> M["End"]
F --> M
%% JSON path
H --> N["Open filename for write"]
N --> O["Use json module to dump data with indent 4"]
O --> P["Close file"]
P --> M
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider making
export_to_csvbehavior explicit for emptydata(e.g., create an empty file with just a header, or raise a clear error) rather than silently returning, so callers can rely on a predictable outcome. - It may be useful for both
export_to_csvandexport_to_jsonto accept a file-like object in addition to a filename, which would make them easier to use in memory-only or web/CLI contexts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making `export_to_csv` behavior explicit for empty `data` (e.g., create an empty file with just a header, or raise a clear error) rather than silently returning, so callers can rely on a predictable outcome.
- It may be useful for both `export_to_csv` and `export_to_json` to accept a file-like object in addition to a filename, which would make them easier to use in memory-only or web/CLI contexts.
## Individual Comments
### Comment 1
<location> `pslab/utils/data_export.py:11` </location>
<code_context>
+import csv
+import json
+
+def export_to_csv(data, filename):
+ """
+ data: list of dictionaries
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider adding an explicit encoding when opening the CSV file.
Relying on the platform default encoding can cause failures or inconsistent results on different systems, particularly with non-ASCII data. Please specify an explicit encoding (e.g. `encoding="utf-8"`) for consistent behavior.
```suggestion
with open(filename, "w", newline="", encoding="utf-8") as f:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR introduces a small utility module to export PSLab sensor/sample data to common interchange formats (CSV and JSON) to improve offline logging and analysis workflows.
Changes:
- Added
export_to_csvhelper to write a list of dict records to CSV (path or file-like target). - Added
export_to_jsonhelper to write data structures to JSON (path or file-like target).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| f = open(target, "w", newline="", encoding="utf-8") if is_path else target | ||
|
|
||
| try: | ||
| writer = csv.DictWriter(f, fieldnames=fieldnames or data[0].keys()) |
There was a problem hiding this comment.
csv.DictWriter(..., fieldnames=fieldnames or data[0].keys()) can break when later rows contain keys not present in the first dict (DictWriter defaults to extrasaction='raise'). For sensor records where fields may vary, compute fieldnames from all rows (or set an appropriate extrasaction) to avoid runtime failures or silently dropped data.
| writer = csv.DictWriter(f, fieldnames=fieldnames or data[0].keys()) | |
| # If fieldnames are not provided, collect all keys from all rows to avoid | |
| # failures when later rows contain keys not present in the first row. | |
| if fieldnames is None: | |
| fieldnames = [] | |
| for row in data: | |
| if isinstance(row, dict): | |
| for key in row.keys(): | |
| if key not in fieldnames: | |
| fieldnames.append(key) | |
| writer = csv.DictWriter(f, fieldnames=fieldnames) |
| if not data: | ||
| raise ValueError("No data provided for export.") | ||
|
|
||
| # Determine if target is a path (str) or a file-like object | ||
| is_path = isinstance(target, str) | ||
| f = open(target, "w", newline="", encoding="utf-8") if is_path else target | ||
|
|
||
| try: | ||
| writer = csv.DictWriter(f, fieldnames=fieldnames or data[0].keys()) | ||
| writer.writeheader() | ||
| writer.writerows(data) | ||
| finally: | ||
| if is_path: | ||
| f.close() | ||
|
|
||
| def export_to_json(data, target): | ||
| """ | ||
| Exports data to a JSON file or file-like object. | ||
| """ | ||
| if not data: | ||
| raise ValueError("No data provided for export.") | ||
|
|
There was a problem hiding this comment.
Both exporters raise on empty data, which prevents writing a valid empty JSON array/object and also prevents writing a header-only CSV when fieldnames is provided. Consider allowing empty iterables (and for CSV, requiring/providing fieldnames when data is empty) so exports can represent “no samples yet” cases.
| def export_to_csv(data, target, fieldnames=None): | ||
| """ | ||
| Exports a list of dictionaries to a CSV file or file-like object. | ||
| """ | ||
| if not data: | ||
| raise ValueError("No data provided for export.") | ||
|
|
||
| # Determine if target is a path (str) or a file-like object | ||
| is_path = isinstance(target, str) | ||
| f = open(target, "w", newline="", encoding="utf-8") if is_path else target | ||
|
|
||
| try: | ||
| writer = csv.DictWriter(f, fieldnames=fieldnames or data[0].keys()) | ||
| writer.writeheader() | ||
| writer.writerows(data) | ||
| finally: | ||
| if is_path: | ||
| f.close() | ||
|
|
||
| def export_to_json(data, target): | ||
| """ | ||
| Exports data to a JSON file or file-like object. | ||
| """ | ||
| if not data: | ||
| raise ValueError("No data provided for export.") | ||
|
|
||
| is_path = isinstance(target, str) | ||
| f = open(target, "w", encoding="utf-8") if is_path else target | ||
|
|
||
| try: | ||
| json.dump(data, f, indent=4) | ||
| finally: |
There was a problem hiding this comment.
The PR description mentions “basic pytest-based tests”, but no tests for export_to_csv/export_to_json are included in tests/. Add coverage for: writing to a path vs file-like object, empty data behavior, and handling of varying dict keys/fieldnames so regressions are caught.
| @@ -0,0 +1,38 @@ | |||
| import csv | |||
| import json | |||
| import io | |||
There was a problem hiding this comment.
io is imported but never used in this module; this will trigger unused-import linting and adds noise. Remove the unused import (or use it if you intended to type-check/validate file-like targets).
| import io |
| import json | ||
| import io | ||
|
|
||
| def export_to_csv(data, target, fieldnames=None): |
There was a problem hiding this comment.
pslab/utils is introduced as a new subpackage but there is no pslab/utils/__init__.py. The rest of the repo’s subpackages (e.g., pslab/bus, pslab/external) include __init__.py; adding one here avoids accidental namespace-package behavior and helps ensure consistent packaging/import behavior.
| # Determine if target is a path (str) or a file-like object | ||
| is_path = isinstance(target, str) | ||
| f = open(target, "w", newline="", encoding="utf-8") if is_path else target | ||
|
|
There was a problem hiding this comment.
The current path detection only treats str as a filesystem path; passing a pathlib.Path/os.PathLike will be treated as a file-like object and will fail. Consider accepting os.PathLike (and possibly str | PathLike) for target to match typical Python I/O APIs.
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Thanks for the PR. The idea of exporting sensor samples to CSV and JSON is useful and relevant for data logging and analysis.
Before we can properly review this for merge, a few process and implementation points need to be addressed:
- Please open an issue first and link it correctly
For new functionality like this, we need an issue describing the use case, proposed API, and scope before adding code. Please open an issue and link it from the PR description using standard GitHub keywords, for example:
Fixes #
This helps align expectations and keeps discussion and design decisions in one place.
- Please provide a short screencast
Please attach a short screencast showing the feature working on your screen. For example:
- a small script calling the CSV and JSON export helpers
- creation of the output files
- a quick look at the generated CSV and JSON
This helps us quickly verify the intended usage and behaviour.
- Implementation details to address
- There is an unused import that should be removed.
- A new utils package is introduced, but it needs an init.py for consistency.
- Path handling should support pathlib.Path or os.PathLike, not only str.
- CSV fieldnames are derived from the first row only, which can break if later rows differ. Please handle or document this clearly.
- Tests are missing. Please add tests covering basic usage, empty data, and varying keys.
Once these points are addressed and the issue is linked, we can proceed with a proper maintainer review.
Thanks for the contribution and for taking the time to improve PSLab.
|
Small process note. We have automatic Copilot PR reviews enabled on this repository. These reviews are only triggered if the contributor has GitHub Copilot enabled and an active license on their own account. Please enable Copilot in your GitHub settings if you have access. In many regions, free licenses are available through educational institutions or developer programs. Enabling Copilot helps us speed up the auto review process and reduces manual review overhead for the core team. The team will review your PR. Thank you for your contribution and cooperation. |
2 participants
This PR adds utility functions to export PSLab sensor data
to CSV and JSON formats.
This improves data usability for PSLab users.
Summary by Sourcery
Add utility module for exporting PSLab sensor data to common file formats.
New Features: