-
-
Notifications
You must be signed in to change notification settings - Fork 318
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
base: main
Are you sure you want to change the base?
lib: add grass JSON API #4801
Conversation
Signed-off-by: Nishant Bansal <[email protected]>
Signed-off-by: Nishant Bansal <[email protected]>
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: |
To ensure we only have one JSON library (
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. |
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]>
I made some modifications and also added tests. Please take a look. |
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... |
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! 🙌 |
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 ;) |
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. |
Hi @nilason, |
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. |
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 Also before merging this, could you please go over the headers and update the copyright year and use uppercase Sorry for the delay, it's been a busy period for me as well. |
Signed-off-by: Nishant Bansal <[email protected]>
Signed-off-by: Nishant Bansal <[email protected]>
Pull Request Description
Ref #4697
Purpose:
This PR addresses #4697 by adding the
gparson
wrapper for better JSON handling in GRASS GIS.Changes: