-
-
Notifications
You must be signed in to change notification settings - Fork 327
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
v.to.db: add JSON support #4036
base: main
Are you sure you want to change the base?
Conversation
@cwhite911 Hi! Can you please review this PR? I would like to use the JSON format added here in the v.report module's JSON implementation to keep the implementation simple. |
@kritibirda26 the schema looks good to me, however I'm debating if we should add the measurement unit to the root object. |
If it's documented for now, it won't be breaking to add it at a later time if needed. So if you can't decide after thinking about it for a while, there's a way to keep the PR going. |
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.
@kritibirda26 This is looking good. Please add test for all of the possible output schemas and update the docs to make it clear that output format only works when the -p flag is set.
@cwhite911 can you please take another look at this PR? |
@kritibirda26 Please add additional unit tests to capture the all of the response data types (point, line, polygon, 3d vectors, mixed with the options: Let's also add the measurement unit to the root json object. |
@cwhite911 Should there be at least one test covering each type and each option? asking because it doesn't seem possible to provide multiple options or types to |
Yes, we should add a test to make sure each key value pair is correctly generated for the option. |
@cwhite911 I have added more tests and measurement units to the root of the object. Can you please suggest which maps/commands to use to test query, azimuth, sinuousity and slope? |
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.
Also make sure to avoid the uninitialized warning like the other module.
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 sure what you mean here. Will you remind me of the other module?
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.
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 azimuth is calculated from a line feature. I suggest using the majorroads
layer for the test.
Azimuth
v.to.db -p map=roadsmajor@PERMANENT option=azimuth units=radians format=json
v.to.db -p map=roadsmajor@PERMANENT option=azimuth units=degrees format=json
Sinuous
v.to.db -p map=roadsmajor@PERMANENT option=sinuous format=json
Slope
To test slope I don't think we have a layer in the test dataset the is line feature with z coordinates.
For the test you could create one using v.to.3d
and then testing that with
v.to.db -p map=roadsmajor3d option=slope units=degrees format=json
@cwhite911, Hi! I have updated the PR with tests for azimuth and sinuous. For slope, I tried to create a 3d map but failed. I also followed other examples of creating a 3d dataset from 2d (from the tests of other modules) but the slope always comes to be 0 in both plain and json mode. |
Add JSON output support to v.to.db module. The output looks like as follows: