fix: use elif in _write_data_file and _read_data_file to avoid redundant checks#570
fix: use elif in _write_data_file and _read_data_file to avoid redundant checks#57031groot wants to merge 2 commits into
Conversation
…ant checks Signed-off-by: Parv Agrawal <agrawalparv13@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 31groot The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Code Review
This pull request updates the dataset helper methods _write_data_file and _read_data_file in core/testenvmanager/dataset/dataset.py to use elif instead of sequential if statements when checking for the CSV data format. The reviewer recommends adding else blocks to both methods to raise a ValueError for unsupported data formats, which prevents silent failures and improves error handling.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| elif data_format == DatasetFormat.CSV.value: | ||
| data.to_csv(data_file, index=None) |
There was a problem hiding this comment.
While changing the sequential if to elif is a good step, if data_format is an unsupported format (such as JSON or JSONFORLLM), this method will silently do nothing. It is safer to add an else block that raises a ValueError to prevent silent failures and make debugging easier.
| elif data_format == DatasetFormat.CSV.value: | |
| data.to_csv(data_file, index=None) | |
| elif data_format == DatasetFormat.CSV.value: | |
| data.to_csv(data_file, index=None) | |
| else: | |
| raise ValueError(f"Unsupported data format for writing: {data_format}") |
| elif data_format == DatasetFormat.CSV.value: | ||
| data = pd.read_csv(data_file) |
There was a problem hiding this comment.
Similarly, if data_format is unsupported, this method will silently return None, which will lead to downstream errors like TypeError when other methods attempt to process the returned data. Adding an else block to raise a ValueError provides defensive programming and clearer error messages.
| elif data_format == DatasetFormat.CSV.value: | |
| data = pd.read_csv(data_file) | |
| elif data_format == DatasetFormat.CSV.value: | |
| data = pd.read_csv(data_file) | |
| else: | |
| raise ValueError(f"Unsupported data format for reading: {data_format}") |
…e data file Signed-off-by: Parv Agrawal <agrawalparv13@gmail.com>
48d2caa to
200a61c
Compare
|
/assign @jaypume |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
_write_data_fileand_read_data_fileindataset.pyused sequentialifstatements for mutually exclusivedata_formatchecks. Sincedata_formatcan only match one value at a time, the secondifblockwas always evaluated unnecessarily. Changed to
elifto make the intentexplicit and avoid redundant condition checks.
Which issue(s) this PR fixes:
Fixes #563