Skip to content
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

r.horizon: add json output #3534

Merged
merged 3 commits into from
Mar 26, 2024
Merged

r.horizon: add json output #3534

merged 3 commits into from
Mar 26, 2024

Conversation

petrasovaa
Copy link
Contributor

This PR adds JSON format to r.horizon. Adds option format which can be plain (default, the previous output format) or json.

r.horizon elevation=elevation@PERMANENT direction=50 step=50 coordinates=632431.8242769501,222607.44281332166 format=json
[
    {
        "x": 632431.824277,
        "y": 222607.442813,
        "azimuth": [
            50.000000,
            100.000000,
            150.000000,
            200.000000,
            250.000000,
            300.000000,
            350.000000
        ],
        "horizon_height": [
            0.031715,
            0.065790,
            0.084245,
            0.084245,
            0.008515,
            0.000000,
            0.000000
        ]
    }
]

I am open to alternative JSON schemes if others think they would be better. For the context, I plan to add support for multiple points.

@github-actions github-actions bot added raster Related to raster data processing Python Related code is in Python C Related code is in C module tests Related to Test Suite labels Mar 25, 2024
@petrasovaa petrasovaa self-assigned this Mar 25, 2024
nilason
nilason previously approved these changes Mar 26, 2024
Copy link
Contributor

@nilason nilason left a comment

Choose a reason for hiding this comment

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

Over all this looks fine to me. I have a couple of suggestions, which you may certainly dismiss if you like as they are only style related. Your, in my opinion, good choice of using enum for type of format, begs to use switch statement instead of if..else. In addition to a clearer, albeit a bit more wordy, code, it has the advantage of being checked by the compiler, should the format options change.

raster/r.horizon/main.c Outdated Show resolved Hide resolved
raster/r.horizon/main.c Outdated Show resolved Hide resolved
raster/r.horizon/main.c Outdated Show resolved Hide resolved
@petrasovaa
Copy link
Contributor Author

Over all this looks fine to me. I have a couple of suggestions, which you may certainly dismiss if you like as they are only style related. Your, in my opinion, good choice of using enum for type of format, begs to use switch statement instead of if..else. In addition to a clearer, albeit a bit more wordy, code, it has the advantage of being checked by the compiler, should the format options change.

I was mostly stealing from v.db.select implementation:) I can change it to switch, it's just I forgot it even existed since it was not in Python (looks like it is there now).

@nilason nilason mentioned this pull request Mar 26, 2024
@neteler neteler added this to the 8.4.0 milestone Mar 26, 2024
@petrasovaa petrasovaa merged commit 2869b8a into OSGeo:main Mar 26, 2024
26 checks passed
@petrasovaa petrasovaa deleted the r.horizon-json branch March 26, 2024 19:43
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 Python Related code is in Python raster Related to raster data processing tests Related to Test Suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants