Skip to content

Conversation

@Blake-Madden
Copy link
Contributor

@Blake-Madden Blake-Madden commented Sep 2, 2025

Deprecate older function names
Improve documentation
Adds unit tests
Fix a few minor issues discovered from unit tests
Make Add() overload that can be unsafe private
Closes #9
Closes #2
Fixes part of #10

Deprecate older function names
#closes hosannahighertech#9
@Blake-Madden Blake-Madden changed the title Add API for property accessors Add API for "AsValue()" type functions Sep 5, 2025
@mtangoo
Copy link
Contributor

mtangoo commented Sep 5, 2025

Can you add operator overloading [] as the linked ticket says? Also I will wait for tests that should cover this (and may be the other [] part?) before merging

Copy link
Contributor

@mtangoo mtangoo left a comment

Choose a reason for hiding this comment

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

We need to have an overloaded []

@Blake-Madden
Copy link
Contributor Author

Got it. I'll add [] this weekend. Might need a few days for the tests, I'll try to have those done mid-next week.

@mtangoo
Copy link
Contributor

mtangoo commented Sep 5, 2025

Take your time.
Thanks for good work on the library

This should not be public because this is actually copying what the node is pointing to, not just its content. It is easy to cause a circular reference if you don't know what you are doing and clients should no need to call a low-level function like this anyway.
If you request an array from strings, but the property contains numbers, then the returned vector will now be empty.
@Blake-Madden
Copy link
Contributor Author

I have implemented everything except for operator[].

So, I tried doing this, but the API winds up being really awkward because we use wxSharedPtr for nodes. Because of that, you would need to deference that to use []. As an example:

(*json)["the-property"]->AsString()

IMO, that looks more confusing than json->GetProperty("the-property")->AsString().

What do you think?

@mtangoo
Copy link
Contributor

mtangoo commented Sep 29, 2025

Hi @Blake-Madden thanks for great working you are doing. Let me know when you are done for full review and merging

@Blake-Madden
Copy link
Contributor Author

This is ready now. I've added every unit test that I can think of and the AsXXX() API works great.

The operator[] functionality just doesn't make sense for this library since it uses smart pointers, so I'm leaving that out.

@mtangoo
Copy link
Contributor

mtangoo commented Sep 29, 2025

Thanks for your time and effort. I'll definitely review and merge

Does the same thing, but is much cleaner and easier to read.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Streamline API for property access Write unit tests

2 participants