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

lib: add grass JSON API #4801

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

NishantBansal2003
Copy link
Contributor

Pull Request Description

Ref #4697

Purpose:

This PR addresses #4697 by adding the gparson wrapper for better JSON handling in GRASS GIS.

Changes:

  • Added thin wrapper of the parson with a unique GRASS API.
  • Added unit tests to cover all major operations.

Signed-off-by: Nishant Bansal <[email protected]>
Signed-off-by: Nishant Bansal <[email protected]>
@github-actions github-actions bot added C Related code is in C HTML Related code is in HTML libraries docs labels Dec 4, 2024
@nilason
Copy link
Contributor

nilason commented Dec 5, 2024

Sorry for being slow on response!

What you have come up with looks promising. But before we go into detail, there is structural change I'd prefer we should pursue. As this now stands, we will end up with two json libraries: parson.so and gparson.so. It would be better to end up having only one, the latter. Do you think of a possible solution to this, without changing code that now links to parson.so? Phasing out the direct use of parson API may then be made module by module.

@NishantBansal2003
Copy link
Contributor Author

To ensure we only have one JSON library (gparson) while maintaining backward compatibility, the possible solutions from my understanding are:

  • We’ll keep parson internal to gparson, ensuring it is only used by gparson. Other modules will interact with gparson directly, preventing any direct access to parson from other modules. This can be achieved by making parson a static library, accessible only via gparson.
  • Initially, we’ll retain the parson API for compatibility, but over time, the goal is to make sure that parson is only used by gparson, and all other modules rely on gparson directly.

This solution is based on my current understanding, and there may be better approaches. I welcome any input or guidance from other contributors on this.

@nilason
Copy link
Contributor

nilason commented Dec 5, 2024

A possible way may be to (1) include/add the "gparson" API on top/together with current parson; (2) convert current user code to use gparson API; and (3) drop/rename current parson library to gparson (original parson API will unavoidably be accessible, but not encouraged–not part of GRASS API–a situation we will end up with in any case).

Perhaps gjson would be a better name for GRASS json API library, thoughts anyone? Anonymising the backbone library, for future eventual freedom to change.

Signed-off-by: Nishant Bansal <[email protected]>
@NishantBansal2003
Copy link
Contributor Author

I made some modifications and also added tests. Please take a look.
cc: @nilason

@nilason
Copy link
Contributor

nilason commented Dec 9, 2024

I like the direction this is going (and I tested with an updated version of #4665, which passed on Mac and Win runner). Before moving forward–and to avoid doing changes back-and-forth–let us hear out the opinion of other dev team mates. In particular on the question of unit tests in library: should they be in-compilation tests (like lib/vector/diglib or lib/cdhc) or post-compilation tests (like lib/raster3d)?

@NishantBansal2003
Copy link
Contributor Author

I like the direction this is going (and I tested with an updated version of #4665, which passed on Mac and Win runner). Before moving forward–and to avoid doing changes back-and-forth–let us hear out the opinion of other dev team mates. In particular on the question of unit tests in library: should they be in-compilation tests (like lib/vector/diglib or lib/cdhc) or post-compilation tests (like lib/raster3d)?

Can you provide some opinions here...
cc: @echoix @cwhite911 @petrasovaa @wenzeslaus

@NishantBansal2003
Copy link
Contributor Author

I like the direction this is going (and I tested with an updated version of #4665, which passed on Mac and Win runner). Before moving forward–and to avoid doing changes back-and-forth–let us hear out the opinion of other dev team mates. In particular on the question of unit tests in library: should they be in-compilation tests (like lib/vector/diglib or lib/cdhc) or post-compilation tests (like lib/raster3d)?

Can you provide some opinions here... cc: @echoix @cwhite911 @petrasovaa @wenzeslaus

Hi all, just following up to check if there are any thoughts or preferences regarding this. Let me know if there’s anything I should adjust or address before proceeding. Thanks! 🙌

@echoix
Copy link
Member

echoix commented Dec 16, 2024

I don't know a lot about in-compilation tests. Usually projects like to have them as a separate step, as this testing time at compilation is not always relevant once distributed.

You might have less help with in-compilation tests, or the persons that know about it here are less frequent or have contributed longer ago.

For in-compilation tests, you would need to be careful in order to make sure that all dependencies are already compiled (by dependencies I mean makefile targets). But also, we are working towards using CMake, albeit slowly, but it will have to be ported their too when we will get at it.

Since I know more about what you call post-compilation tests, obviously it would be my choice. But if you have something that justifies one or the other, knowing the possible drawbacks mentionned, the best solution could be one or the other ;)

@NishantBansal2003
Copy link
Contributor Author

I researched the differences between post-compilation and in-compilation testing and found that I prefer post-compilation testing, especially since most libraries already follow this approach. With our gradual move toward CMake, post-compilation tests are also more adaptable to future transitions, avoiding unnecessary complexities.

That said, if any of the developers have strong points in favor of in-compilation tests, I’d be happy to learn about them and consider implementing those as well.

@NishantBansal2003
Copy link
Contributor Author

Hi @nilason,
Could you kindly share your suggestions or preferences regarding any additional changes I could make to the current implementation?

@nilason
Copy link
Contributor

nilason commented Dec 20, 2024

Hi @nilason, Could you kindly share your suggestions or preferences regarding any additional changes I could make to the current implementation?

I have currently little time these days... The wrapper part is quite straightforward and uncontroversial. If you put the test part on the shelf, we can get this through sooner.

@petrasovaa
Copy link
Contributor

I am not an expert, the testing approach you have there seems good to me, to run the actual test in CI, I believe you need to add the actual test similarly to lib/raster3d/testsuite/raster3d_lib_test.py.

Also before merging this, could you please go over the headers and update the copyright year and use uppercase GRASS, and add yourself as the author as appropriate?

Sorry for the delay, it's been a busy period for me as well.

Signed-off-by: Nishant Bansal <[email protected]>
@github-actions github-actions bot added Python Related code is in Python tests Related to Test Suite labels Dec 21, 2024
Signed-off-by: Nishant Bansal <[email protected]>
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 docs HTML Related code is in HTML libraries Python Related code is in Python tests Related to Test Suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants