-
-
Notifications
You must be signed in to change notification settings - Fork 362
v.db.connect: Add JSON support #6077
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nishant Bansal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going deeper into this, I now see that #6014 does not capture all the complexity.
vector/v.db.connect/main.c
Outdated
G_json_object_set_string( | ||
conn_object, "type", | ||
db_sqltype_name(db_get_column_sqltype( | ||
db_get_table_column(table, col)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I like the simplicity of "name" and "type" suggested in #6014, this output would be likely used in a way where where SQL context is not the only context, e.g., translating between Python and SQL worlds. Hence, in v.db.select, I used sql_type and is_number for column info. Internally in C, we have also corresponding C type which we could output, but we don't at this point, but we could have something like c_type.
Anyway, sql_type was then used with v.info -c
and db.columns. So, for consistency, the output should look the same as for v.info -c
.
vector/v.db.connect/main.c
Outdated
if (fi->name) | ||
G_json_object_set_string(conn_object, "name", | ||
fi->name); | ||
else | ||
G_json_object_set_string(conn_object, "name", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, use "layer_name". The underlying struct [field_info](https://grass.osgeo.org/programming8/structfield__info.html)
has attributes number
and name
. Simple layer
is sort of expected everywhere and as input parameter, so layer
should be there. While may be a number or a string as input, but having it as the number is not wrong and should be usable in most contexts. When we take the name as a extra information (it is documented as optional in field_info
struct and the same is implied in this code, then having a longer name for it makes sense to me.
vector/v.db.connect/main.c
Outdated
if (fi->name) | ||
G_json_object_set_string(conn_object, "name", | ||
fi->name); | ||
else | ||
G_json_object_set_string(conn_object, "name", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use null
for missing value, not empty string. This better expresses that it is missing as opposed to set to something which happened to be empty.
fprintf(stdout, _("Vector map <%s> is connected by:\n"), | ||
input); | ||
} | ||
for (i = 0; i < num_dblinks; i++) { | ||
if ((fi = Vect_get_dblink(&Map, i)) == NULL) | ||
G_fatal_error(_("Database connection not defined")); | ||
|
||
if (shell_print->answer) { | ||
switch (format) { | ||
case CSV: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not excited about that, but to have the new take on the CSV format consistent with JSON, we should include CSV header and separate layer number from layer name.
To implement this, CSV needs to have two variants, the backwards compatible one which will be triggered by the flags and the new one which will be triggered by the format option. The backwards compatible one would be removed in the future for v9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same would apply to -c
, i.e., header + backwards compatibility variant. (Also implying that v.info -c format=shell
is now wrongly labeled as shell, not CSV and needs the same header+compatibility update.) What do you think @NishantBansal2003 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the best I could think of as of now, so let's go this way instead.
let's open an issue for v.info
to track this as well? will look into it later on.
vector/v.db.connect/main.c
Outdated
csv_print->label = _("Print all map connection parameters in csv " | ||
"style and exit [deprecated]"); | ||
csv_print->description = _( | ||
"Format: layer[/layer name] table key database driver" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While not critical, let's be clear about the current functionality. The original description is wrong in terms of separator. Maybe the default separator changed over time or maybe it is meant to indicate only order, not format. Space seems more readable, but saying Order:
would be necessary. Or use |
and keep Format:
.
vector/v.db.connect/main.c
Outdated
csv_print->label = _("Print all map connection parameters in csv " | ||
"style and exit [deprecated]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think CSV should be all caps and I'm not a fan of "...and exit". What about simpler:
csv_print->label = _("Print all map connection parameters in csv " | |
"style and exit [deprecated]"); | |
csv_print->label = _("Print all map connection parameters in a legacy format [deprecated]"); |
Signed-off-by: Nishant Bansal <[email protected]>
Fixes: #6014
This PR adds JSON support to the
v.db.connect
module. The JSON output looks like:-p
flag:-c
flag:This PR includes the following changes:
format
option withplain
,csv
, andjson
modes for output formatting.format = csv
option; the-g
flag is now deprecated.-p
flag, and CSV for the-g
and-c
flags.