v.info: Add JSON output for history #4989

NishantBansal2003 · 2025-01-25T09:02:58Z

Fixes: #4217
Use parson to add json output format support to the v.info module for history flag.
The JSON output looks like as follows:

$ grass --exec v.info -h map=roadsmajor format=json
{
    "records": [
        {
            "command": "v.in.ogr dsn=\"majorroads.shp\" output=\"roads_major\" min_area=0.0001 snap=-1",
            "mapset_path": "\/bigdata\/grassdata05\/wakestpfeet\/PERMANENT",
            "user": "helena",
            "date": "Tue Nov  7 18:34:20 2006"
        },
        {
            "command": "v.proj input=\"wroads_major\" location=\"wakestpfeet\" output=\"roadsmajor_wake\"",
            "mapset_path": "\/bigdata\/grassdata05\/ncfromfile\/PERMANENT",
            "user": "helena",
            "date": "Wed Nov  8 00:18:50 2006"
        },
        {
            "command": "v.db.connect -o map=\"roadsmajor@PERMANENT\" driver=\"sqlite\" database=\"\/home\/neteler\/grassdata\/nc_spm_latest\/nc_spm_08\/PERMANENT\/sqlite\/sqlite.db\" table=\"roadsmajor\" key=\"cat\" layer=\"1\" separator=\"|\"",
            "mapset_path": "\/home\/neteler\/grassdata\/nc_spm_latest\/nc_spm_08\/PERMANENT",
            "user": "neteler",
            "date": "Mon Nov 26 16:55:38 2012"
        },
        {
            "command": "v.db.connect -o map=\"roadsmajor@PERMANENT\" driver=\"sqlite\" database=\"$GISDBASE\/$LOCATION_NAME\/$MAPSET\/sqlite\/sqlite.db\" table=\"roadsmajor\" key=\"cat\" layer=\"1\" separator=\"|\"",
            "mapset_path": "\/home\/neteler\/grassdata\/nc_spm_08_grass7\/PERMANENT",
            "user": "neteler",
            "date": "Fri Dec  7 23:25:12 2012"
        }
    ]
}

Signed-off-by: Nishant Bansal <[email protected]>

vector/v.info/print.c

Signed-off-by: Nishant Bansal <[email protected]>

NishantBansal2003 · 2025-01-27T07:32:05Z

Failing gunittest tests on the macOS build: FAILED ./scripts/g.extension/testsuite/test_addons_download.py (1 test failed). I am unsure why this test is failing.

Previously in the macOS CI, test_github_official_module_man_page_src_code_links_exists was failing.
Now, test_github_install is failing inside ./scripts/g.extension/testsuite/test_addons_download.py.

neteler · 2025-01-27T08:16:55Z

macOS: I have restarted this test.

And the OSGeo4W CI error is a bit unexpected:

checking whether to use PostgreSQL... "yes"
checking for location of PostgreSQL includes... /c/OSGeo4W/include
checking for libpq-fe.h... yes
checking for location of PostgreSQL library... /c/OSGeo4W/lib
checking for PQsetdbLogin in -lpq... no
checking for PQsetdbLogin in -lpq... no
checking for PQsetdbLogin in -lpq... no
checking for PQsetdbLogin in -lpq... no
configure: error: *** Unable to locate PostgreSQL library.

nilason · 2025-01-27T08:20:30Z

And the OSGeo4W CI error is a bit unexpected:

See #4990

wenzeslaus

This looks good overall and the generated JSON seems good too. I have just comments about code structure.

I'm still for the mapset_path but I would be happy to discuss alternatives if anyone feels like.

I completely agree that a function is needed. With how the code is written, two separate functions seem to be more fitting. One for actual parsing and another one for JSON. Let's say we would want to write to a different format than JSON. However, my reason for wanting two functions is mostly from code reading perspective.

wenzeslaus · 2025-02-05T03:34:25Z

vector/v.info/parse.c

+        JSON_Value *info_value = json_value_init_object();
+        if (info_value == NULL) {
+            G_fatal_error(
+                _("Failed to initialize JSON object. Out of memory?"));
+        }
+        JSON_Object *info_object = json_object(info_value);
+
+        json_object_set_string(info_object, "command", command);
+        snprintf(mapset_path, MAX_STR_LEN, "%s/%s/%s", gisdbase, location,
+                 mapset);
+        json_object_set_string(info_object, "mapset_path", mapset_path);
+        json_object_set_string(info_object, "user", user);
+        json_object_set_string(info_object, "date", date);
+
+        json_array_append_value(record_array, info_value);


This is the second function which adds one record to JSON.

wenzeslaus · 2025-02-05T03:35:19Z

vector/v.info/parse.c

+        json_object_set_string(info_object, "command", command);
+        snprintf(mapset_path, MAX_STR_LEN, "%s/%s/%s", gisdbase, location,


command and gisdbase are here inputs while there were outputs the previous calls of the function.

wenzeslaus · 2025-02-05T03:36:14Z

vector/v.info/parse.c

+            G_fatal_error(
+                _("Failed to initialize JSON object. Out of memory?"));
+        }
+        JSON_Object *info_object = json_object(info_value);


This assumes the order in the source file and in the functions calls. Perhaps safe assumption, but confusing in the context.

wenzeslaus · 2025-02-05T03:43:46Z

vector/v.info/parse.c

+void parse_history_json(char *buf, char *command, char *gisdbase,
+                        char *location, char *mapset, char *user, char *date,
+                        char *mapset_path, JSON_Array *record_array)


The function ended up here just because of its name. The parsing here is parsing command line input. I don't

In the tool itself, the function is more linked to the print anyway, so just keep it there for simplicity. Actually, in the current form, it is also very much linked to that print code.

In any case, please document split the function into two and document them with a sentence or two. It took me some time to orient myself in this code although it is actually straightforward and nicely written otherwise.

Signed-off-by: Nishant Bansal <[email protected]>

NishantBansal2003 · 2025-02-05T10:31:07Z

Hey @wenzeslaus, thanks for the detailed review. I've tried to improve the code structure based on your feedback and will keep it in mind for next time. Could you please take a look and check if everything is covered? Thanks.

nilason

After a quick reading, I have some minor suggestions.

vector/v.info/print.c

vector/v.info/local_proto.h

Signed-off-by: Nishant Bansal <[email protected]>

vector/v.info/print.c

Co-authored-by: Nicklas Larsson <[email protected]> Signed-off-by: Nishant Bansal <[email protected]>

cwhite911

Nice job @NishantBansal2003

nilason · 2025-02-05T17:32:32Z

I wonder, with the traditional output the history is ordered, this is not evident with current json output?!

Update: e.g. add a "history_no", or "event_no" or similarly named int to json output.

NishantBansal2003 · 2025-02-06T08:38:05Z

I wonder, with the traditional output the history is ordered, this is not evident with current json output?!

The order of elements within a JSON object created by Parson may vary (i.e., they are not necessarily in the order in which we inserted them). However, the order of elements in a JSON list/array created by Parson remains the same (i.e., exactly in the order we inserted them) unless you remove elements. See: kgabis/parson#218

nilason · 2025-02-06T08:58:46Z

I wonder, with the traditional output the history is ordered, this is not evident with current json output?!

The order of elements within a JSON object created by Parson may vary (i.e., they are not necessarily in the order in which we inserted them). However, the order of elements in a JSON list/array created by Parson remains the same (i.e., exactly in the order we inserted them) unless you remove elements. See: kgabis/parson#218

That is what I mean. The order of history is important, but this information is lost working with a json file with an array of "records" (no predictable way to know the order there, is there). It is like a database table without a pkey.

Something like:

{
    "records": [
        {
            "history_no_or_whatever_name": "1",
            "command": "v.in.ogr dsn=\"majorroads.shp\" output=\"roads_major\" min_area=0.0001 snap=-1",
            "mapset_path": "\/bigdata\/grassdata05\/wakestpfeet\/PERMANENT",
            "user": "helena",
            "date": "Tue Nov  7 18:34:20 2006"
        },
...

This is, I imagine, easy to implement with a counter in the read line loop.

Signed-off-by: Nishant Bansal <[email protected]>

vector/v.info/testsuite/test_vinfo.py

wenzeslaus

Given the strings are used to signal the state, I think it is better to clear them in the caller code instead of the function. It will be more clear what is happening.

nilason · 2025-02-06T13:32:50Z

Given the strings are used to signal the state, I think it is better to clear them in the caller code instead of the function. It will be more clear what is happening.

I'm not sure what you mean.

nilason

Looks good to me.
@NishantBansal2003 Thanks!

Signed-off-by: Nishant Bansal <[email protected]>

NishantBansal2003 · 2025-02-06T18:31:06Z

Given the strings are used to signal the state, I think it is better to clear them in the caller code instead of the function. It will be more clear what is happening.

Sorry, I'm not entirely clear on your suggestion. Could you please explain in more detail where you think the strings should be cleared?

petrasovaa · 2025-02-07T22:06:19Z

Given the strings are used to signal the state, I think it is better to clear them in the caller code instead of the function. It will be more clear what is happening.

I'm not sure what you mean.

I think he refers to parse_history_line

nilason · 2025-02-07T22:11:11Z

Given the strings are used to signal the state, I think it is better to clear them in the caller code instead of the function. It will be more clear what is happening.

I'm not sure what you mean.

I think he refers to parse_history_line

That would be my guess too, but that’s where it is known when to clear.

nilason · 2025-02-07T22:23:21Z

That would be my guess too, but that’s where it is known when to clear.

Or possibly after call to add_record_to_json(), would work as well.

wenzeslaus · 2025-02-07T22:40:12Z

That would be my guess too, but that’s where it is known when to clear.

Or possibly after call to add_record_to_json(), would work as well.

Yes, that's what I meant. Keeping the state out of the function. Sorry for not being clear.

echoix · 2025-02-07T23:22:15Z

So is this PR ok, or your requested changes still apply?

vector/v.info/print.c

Signed-off-by: Nishant Bansal <[email protected]>

v.info: Add JSON output for history

1b3ebea

Signed-off-by: Nishant Bansal <[email protected]>

github-actions bot added vector Related to vector data processing Python Related code is in Python C Related code is in C module tests Related to Test Suite labels Jan 25, 2025

remove debug statements

fcc305a

Signed-off-by: Nishant Bansal <[email protected]>

cwhite911 suggested changes Jan 26, 2025

View reviewed changes

vector/v.info/print.c Outdated Show resolved Hide resolved

vector/v.info/print.c Outdated Show resolved Hide resolved

NishantBansal2003 added 3 commits January 26, 2025 11:01

Parse history into a separate function

ebc5d7f

Signed-off-by: Nishant Bansal <[email protected]>

fixes length in snprintf call

5467c26

Signed-off-by: Nishant Bansal <[email protected]>

fixes variable declarations

1c90fdf

Signed-off-by: Nishant Bansal <[email protected]>

NishantBansal2003 requested a review from cwhite911 January 28, 2025 04:29

petrasovaa and others added 2 commits January 28, 2025 10:49

Merge branch 'main' into v-info-hist

3e89a66

Merge branch 'main' into v-info-hist

50ef1f2

wenzeslaus requested changes Feb 5, 2025

View reviewed changes

apply suggestions from code review

3c5f905

Signed-off-by: Nishant Bansal <[email protected]>

nilason requested changes Feb 5, 2025

View reviewed changes

vector/v.info/print.c Outdated Show resolved Hide resolved

vector/v.info/print.c Outdated Show resolved Hide resolved

vector/v.info/local_proto.h Outdated Show resolved Hide resolved

vector/v.info/local_proto.h Outdated Show resolved Hide resolved

NishantBansal2003 added 2 commits February 5, 2025 18:02

fixes CI issues

c421cc8

Signed-off-by: Nishant Bansal <[email protected]>

fixes input string processing

9619830

Signed-off-by: Nishant Bansal <[email protected]>

nilason reviewed Feb 5, 2025

View reviewed changes

vector/v.info/print.c Outdated Show resolved Hide resolved

apply changes based on code review

c5428ad

Co-authored-by: Nicklas Larsson <[email protected]> Signed-off-by: Nishant Bansal <[email protected]>

nilason self-requested a review February 5, 2025 15:01

cwhite911 approved these changes Feb 5, 2025

View reviewed changes

use history_number in json output

cda87f2

Signed-off-by: Nishant Bansal <[email protected]>

nilason reviewed Feb 6, 2025

View reviewed changes

vector/v.info/testsuite/test_vinfo.py Outdated Show resolved Hide resolved

wenzeslaus requested changes Feb 6, 2025

View reviewed changes

nilason previously approved these changes Feb 6, 2025

View reviewed changes

fixes comments in test_vinfo

bb2307f

Signed-off-by: Nishant Bansal <[email protected]>

NishantBansal2003 dismissed nilason’s stale review via bb2307f February 6, 2025 18:20

NishantBansal2003 requested a review from nilason February 6, 2025 18:31

nilason previously approved these changes Feb 6, 2025

View reviewed changes

nilason reviewed Feb 7, 2025

View reviewed changes

vector/v.info/print.c Outdated Show resolved Hide resolved

apply changes based on review

11cf8ba

Signed-off-by: Nishant Bansal <[email protected]>

NishantBansal2003 dismissed nilason’s stale review via 11cf8ba February 8, 2025 04:13

NishantBansal2003 requested review from wenzeslaus and nilason February 8, 2025 04:14

nilason approved these changes Feb 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v.info: Add JSON output for history #4989

v.info: Add JSON output for history #4989

NishantBansal2003 commented Jan 25, 2025

NishantBansal2003 commented Jan 27, 2025 •

edited

Loading

neteler commented Jan 27, 2025

nilason commented Jan 27, 2025

wenzeslaus left a comment

wenzeslaus Feb 5, 2025

wenzeslaus Feb 5, 2025

wenzeslaus Feb 5, 2025

wenzeslaus Feb 5, 2025

NishantBansal2003 commented Feb 5, 2025

nilason left a comment

cwhite911 left a comment

nilason commented Feb 5, 2025 •

edited

Loading

NishantBansal2003 commented Feb 6, 2025

nilason commented Feb 6, 2025

wenzeslaus left a comment

nilason commented Feb 6, 2025

nilason left a comment

NishantBansal2003 commented Feb 6, 2025

petrasovaa commented Feb 7, 2025

nilason commented Feb 7, 2025

nilason commented Feb 7, 2025

wenzeslaus commented Feb 7, 2025

echoix commented Feb 7, 2025

		json_object_set_string(info_object, "command", command);
		snprintf(mapset_path, MAX_STR_LEN, "%s/%s/%s", gisdbase, location,

v.info: Add JSON output for history #4989

Are you sure you want to change the base?

v.info: Add JSON output for history #4989

Conversation

NishantBansal2003 commented Jan 25, 2025

NishantBansal2003 commented Jan 27, 2025 • edited Loading

neteler commented Jan 27, 2025

nilason commented Jan 27, 2025

wenzeslaus left a comment

Choose a reason for hiding this comment

wenzeslaus Feb 5, 2025

Choose a reason for hiding this comment

wenzeslaus Feb 5, 2025

Choose a reason for hiding this comment

wenzeslaus Feb 5, 2025

Choose a reason for hiding this comment

wenzeslaus Feb 5, 2025

Choose a reason for hiding this comment

NishantBansal2003 commented Feb 5, 2025

nilason left a comment

Choose a reason for hiding this comment

cwhite911 left a comment

Choose a reason for hiding this comment

nilason commented Feb 5, 2025 • edited Loading

NishantBansal2003 commented Feb 6, 2025

nilason commented Feb 6, 2025

wenzeslaus left a comment

Choose a reason for hiding this comment

nilason commented Feb 6, 2025

nilason left a comment

Choose a reason for hiding this comment

NishantBansal2003 commented Feb 6, 2025

petrasovaa commented Feb 7, 2025

nilason commented Feb 7, 2025

nilason commented Feb 7, 2025

wenzeslaus commented Feb 7, 2025

echoix commented Feb 7, 2025

NishantBansal2003 commented Jan 27, 2025 •

edited

Loading

nilason commented Feb 5, 2025 •

edited

Loading