Add CSV and JSON export utilities for PSLab sensor data #261
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds pytest-based tests around new CSV and JSON data export utilities for PSLab sensor data, including file creation checks and validation for empty input data. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Author
|
This is my first contribution to PSLab. |
![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 2 issues, and left some high level feedback:
- Consider defining and enforcing a consistent behavior for empty input across both
export_to_csvandexport_to_json(e.g., both raisingValueErroror both producing empty files), so callers don’t have to handle each exporter differently. - It may be useful for
export_to_csvandexport_to_jsonto accept file-like objects in addition to file paths, which would make them easier to integrate with in-memory streams and higher-level APIs. - Clarify and standardize how the exporters handle records with heterogeneous or missing keys (e.g., column ordering, default values), and ensure the implementation consistently applies that strategy.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider defining and enforcing a consistent behavior for empty input across both `export_to_csv` and `export_to_json` (e.g., both raising `ValueError` or both producing empty files), so callers don’t have to handle each exporter differently.
- It may be useful for `export_to_csv` and `export_to_json` to accept file-like objects in addition to file paths, which would make them easier to integrate with in-memory streams and higher-level APIs.
- Clarify and standardize how the exporters handle records with heterogeneous or missing keys (e.g., column ordering, default values), and ensure the implementation consistently applies that strategy.
## Individual Comments
### Comment 1
<location> `tests/test_data_export.py:4-8` </location>
<code_context>
+import pytest
+from pslab.utils.data_export import export_to_csv, export_to_json
+
+def test_export_csv_creates_file(tmp_path):
+ data = [{"a": 1, "b": 2}]
+ file_path = tmp_path / "data.csv"
+ export_to_csv(data, str(file_path))
+ assert file_path.exists()
+
+def test_export_json_creates_file(tmp_path):
</code_context>
<issue_to_address>
**suggestion (testing):** CSV export test only checks that the file exists but not that its contents are correct.
This test would still pass if the CSV had wrong headers, delimiters, or malformed rows. Please extend it to read and assert the contents—for example, that the header matches the expected columns, the row count is correct, and at least one data row matches the input—so it verifies the export logic, not just file creation.
Suggested implementation:
```python
import csv
import pytest
from pslab.utils.data_export import export_to_csv, export_to_json
def test_export_csv_creates_file(tmp_path):
data = [{"a": 1, "b": 2}]
file_path = tmp_path / "data.csv"
export_to_csv(data, str(file_path))
# File is created
assert file_path.exists()
# Contents are correct
with file_path.open(newline="") as f:
reader = csv.DictReader(f)
# Header matches expected columns
assert reader.fieldnames == ["a", "b"]
rows = list(reader)
# Row count is correct
assert len(rows) == 1
row = rows[0]
# Row data matches input (compare as strings to be robust)
assert row["a"] == str(data[0]["a"])
assert row["b"] == str(data[0]["b"])
```
```python
def test_export_json_creates_file(tmp_path):
data = [{"x": 10}]
file_path = tmp_path / "data.json"
export_to_json(data, str(file_path))
assert file_path.exists()
```
</issue_to_address>
### Comment 2
<location> `tests/test_data_export.py:10-8` </location>
<code_context>
+ export_to_csv(data, str(file_path))
+ assert file_path.exists()
+
+def test_export_json_creates_file(tmp_path):
+ data = [{"x": 10}]
+ file_path = tmp_path / "data.json"
+ export_to_json(data, str(file_path))
+ assert file_path.exists()
+
+def test_export_empty_data_raises_error():
</code_context>
<issue_to_address>
**suggestion (testing):** JSON export test should validate the serialized content, not just file creation.
Consider mirroring the CSV test by loading the JSON (e.g., with `json.load`) and asserting the parsed structure and values match `data`. This will catch issues with encoding, formatting, or changes to the top-level container type.
Suggested implementation:
```python
import json
import pytest
from pslab.utils.data_export import export_to_csv, export_to_json
```
```python
def test_export_json_creates_file(tmp_path):
data = [{"x": 10}]
file_path = tmp_path / "data.json"
export_to_json(data, str(file_path))
assert file_path.exists()
with file_path.open("r", encoding="utf-8") as f:
loaded = json.load(f)
assert loaded == data
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
4
to
8
| def test_export_csv_creates_file(tmp_path): | ||
| data = [{"a": 1, "b": 2}] | ||
| file_path = tmp_path / "data.csv" | ||
| export_to_csv(data, str(file_path)) | ||
| assert file_path.exists() |
There was a problem hiding this comment.
suggestion (testing): CSV export test only checks that the file exists but not that its contents are correct.
This test would still pass if the CSV had wrong headers, delimiters, or malformed rows. Please extend it to read and assert the contents—for example, that the header matches the expected columns, the row count is correct, and at least one data row matches the input—so it verifies the export logic, not just file creation.
Suggested implementation:
import csv
import pytest
from pslab.utils.data_export import export_to_csv, export_to_json
def test_export_csv_creates_file(tmp_path):
data = [{"a": 1, "b": 2}]
file_path = tmp_path / "data.csv"
export_to_csv(data, str(file_path))
# File is created
assert file_path.exists()
# Contents are correct
with file_path.open(newline="") as f:
reader = csv.DictReader(f)
# Header matches expected columns
assert reader.fieldnames == ["a", "b"]
rows = list(reader)
# Row count is correct
assert len(rows) == 1
row = rows[0]
# Row data matches input (compare as strings to be robust)
assert row["a"] == str(data[0]["a"])
assert row["b"] == str(data[0]["b"])def test_export_json_creates_file(tmp_path):
data = [{"x": 10}]
file_path = tmp_path / "data.json"
export_to_json(data, str(file_path))
assert file_path.exists()
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 utilities for exporting PSLab sensor data to common file formats and ensure they are covered by basic tests.
New Features:
Tests: