v.db.select: rewrite JSON output using gjson library#7032
v.db.select: rewrite JSON output using gjson library#7032Abhi-d-gr8 wants to merge 4 commits intoOSGeo:mainfrom
Conversation
github-actions
bot
added
vector
github-actions
bot
added
Python
wenzeslaus
left a comment
wenzeslaus
left a comment
There was a problem hiding this comment.
Since the code is touching format == CSV code, it would be good to test that beyond the one test which is there which is looking at the values but in a way abstracts the format itself. Add a test which is in the same style as testComma, but with format="csv". Attach screenshots which show the test actually runs and also that it fails if you intentionally introduce error into the code.
Similarly, extent test is missing as far as I can tell. We can't merge these without a clear proof these are actually running.
| json.loads(sel.outputs.stdout) | ||
| actual_json = json.loads(sel.outputs.stdout) | ||
| reference_json = json.loads(out_json) | ||
| self.assertEqual(actual_json, reference_json) |
There was a problem hiding this comment.
Leave the assert out of try-except block.
vector/v.db.select/main.c
Outdated
| if (flags.region->answer) { | ||
| /* Extent output - structure will be built later */ | ||
| } | ||
| else { |
vector/v.db.select/main.c
Outdated
| G_json_object_set_value(info_object, "columns", columns_value); | ||
| G_json_object_set_value(root_object, "info", info_value); | ||
|
|
||
| /* Initialize records array */ |
There was a problem hiding this comment.
No comments for obvious code. Eval the others, too.
vector/v.db.select/main.c
Outdated
| /* Note: Parson library handles JSON escaping automatically, | ||
| * so we only escape for non-JSON formats when flag is set | ||
| */ |
There was a problem hiding this comment.
Good to mention in the context. Use period at the end of sentences, leave out "note". We are adopting // for comments as we go.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Hi @wenzeslaus, thanks for the detailed feedback. CSV format path: added testFormatCsv() in vector/v.db.select/testsuite/test_v_db_select.py, written in the same style as testComma, but using format="csv". This verifies the CSV-specific behavior (text fields quoted), so the format == CSV branch is now directly exercised. Extent path: added testJSONExtent() covering -r with format=json, asserting that the output contains an extent object with numeric n/s/w/e keys. To provide clear proof that these tests are actually running and meaningful, I’ve attached three screenshots:
Let me know if you’d like an additional extent test for another output format as well. 
|
2 participants
I've gone ahead and refactored v.db.select so it now uses the official
gjsonlibrary instead of manually building JSON strings withfprintf.This makes the code quite a bit cleaner. I've updated the main attribute records, the column metadata, and the region extent (
-r) logic to use the library. I also made sure to handle an edge case where skipping records with the-fflag could have led to a small memory leak.I verified everything by running the tests in vector/v.db.select/testsuite/test_v_db_select_json_csv.py, and they are all passing perfectly.
Fixes #6969