Skip to content

r.geomorphon: rewrite JSON profile output to use gjson/parson library#7003

Closed
Rhushya wants to merge 5 commits intoOSGeo:mainfrom
Rhushya:main
Closed

r.geomorphon: rewrite JSON profile output to use gjson/parson library#7003
Rhushya wants to merge 5 commits intoOSGeo:mainfrom
Rhushya:main

Conversation

@Rhushya
Copy link
Copy Markdown

@Rhushya Rhushya commented Jan 31, 2026

@echoix @petrasovaa

Summary

Fixes #6970

This PR switches r.geomorphon's JSON profile output from manual string formatting to the GRASS JSON wrapper (gjson/parson), improving maintainability and avoiding comma/indent edge cases by building the output via a proper JSON object model.

Changes

  • Makefile: Added $(PARSONLIB) to LIBES for library linking
  • local_proto.h: Added #include <grass/parson/gjson.h>
  • profile.c: Rewrote write_json() function to use gjson API

Approach

Following the feedback to keep it simple and focus only on JSON:

  1. Kept the existing token array system - YAML and XML output paths remain unchanged
  2. Rewrote only write_json() - Converts from manual fprintf formatting to gjson API
  3. Preserved the external API - All prof_* functions (prof_int, prof_sso, etc.) stay the same
  4. Followed v.what pattern - Used the same gjson wrapper pattern that exists in other GRASS modules

Technical Details

The new implementation:

  • Builds a G_JSON_Object tree from the existing token array
  • Uses G_json_serialize_to_string_pretty() for formatted output
  • Handles NaN values with G_json_object_set_null()
  • Uses a stack-based approach for nested objects (similar to existing XML code)

Copilot AI review requested due to automatic review settings January 31, 2026 05:06
@github-actions github-actions Bot added raster Related to raster data processing C Related code is in C module labels Jan 31, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the JSON profile output in r.geomorphon to use the GRASS gjson/parson library instead of manual string formatting, addressing issue #6970. The change improves maintainability by replacing complex indent and comma management with a proper JSON object model.

Changes:

  • Switched from manual fprintf-based JSON generation to gjson/parson library API
  • Added PARSONLIB dependency to the module's Makefile
  • Rewrote write_json() function to build a JSON object tree and serialize it

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
raster/r.geomorphon/Makefile Added PARSONLIB to link against the parson library
raster/r.geomorphon/local_proto.h Added gjson.h header include for JSON API access
raster/r.geomorphon/profile.c Completely rewrote write_json() to use gjson API instead of manual formatting

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread raster/r.geomorphon/profile.c
Comment thread raster/r.geomorphon/profile.c
@echoix
Copy link
Copy Markdown
Member

echoix commented Jan 31, 2026

@Rhushya i think you need to at least compile it locally before. Assume your PR. Try it out, know its limits. Have tests that prove what was there before, and what you did now are the same (or fixes something that wasn’t there before.

@Rhushya
Copy link
Copy Markdown
Author

Rhushya commented Jan 31, 2026

Hey @echoix,

You're absolutely right - I should've tested this properly before submitting. I went back and compiled everything locally in WSL Ubuntu, and here's what I found:

Build: Compiled successfully against GRASS 8.3.2 with parson library linked

Tests: All passing

  • JSON output: works
  • YAML output: works
  • XML output: works
  • File output: works

Here's a snippet of the JSON output my changes produce:

{
  "final_results": {
    "landform_cat": 4,
    "landform_code": "SH",
    "landform_name": "shoulder",
    "azimuth": 97.85808563
  },
  "map_info": {
    "elevation_name": "test_dem",
    "projection": 3
  },
  "timestamp": "2026-01-31T18:53:35Z",
  "generator": "r.geomorphon GRASS GIS 8.3.2"
}

What I changed:

  • Replaced manual fprintf formatting with gjson API calls
  • Fixed the include path from <grass/parson/gjson.h> to <grass/gjson.h> (this was breaking CI)
  • Kept YAML and XML untouched since the scope was just JSON

The output structure stays exactly the same as before - just cleaner code underneath that's easier to maintain. Same pattern used in v.what and other modules.

@echoix
Copy link
Copy Markdown
Member

echoix commented Jan 31, 2026

Grass 8.3.2 is very very old. And the current master is more than a year ahead of 8.4

@echoix
Copy link
Copy Markdown
Member

echoix commented Jan 31, 2026

And json didn't exist at that time

@wenzeslaus
Copy link
Copy Markdown
Member

Given how r.geomorphon JSON code is written, this requires more carefulness. Also I did not check the test coverage, but I expect a change like this would have to add more tests. The version mismatch in the last comment is questionable. This PR is a candidate for closing.

@Rhushya Rhushya closed this by deleting the head repository Feb 6, 2026
@Rhushya
Copy link
Copy Markdown
Author

Rhushya commented Feb 6, 2026

Hey ill restart as a new issue i had lot of errors in the commits would delete it and start fresh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C Related code is in C module raster Related to raster data processing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feat] Rewrite r.geomorphon JSON output to use parson library

4 participants