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

Json/XML Archive improvement #321

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ChrisBFX
Copy link

Better error messages when loading of nvp fails. You can now see the complete path of nvps that fails which helps if you're parsing big xml/json archives with nested data structures.

The name of the root node in the xml can now be configured on a per archive basis (default is the one set with the CEREAL_XML_STRING_VALUE macro, so no everything should work as before) which is easier than the macro (which is not always possible if you want to use different root names and polymorphism support).

@AzothAmmo
Copy link
Contributor

Can you clarify the NVP exception changes you've made here? We already throw an exception (cereal::Exception) if we can't find the NVP - are you trying to catch other potential errors (I see you catch std::exception).

@ChrisBFX
Copy link
Author

ChrisBFX commented Aug 1, 2016

Yes this catches more errors, f.e. when rapidjson can't parse a value cause of a type-mismatch it throws an exception, so this would give you more info. (Though I'm seeing now you modified rapidjson to throw an cereal::Exception, so i guess it would be ok to change the catch in the json archive to cereal::Exception, the xml archive though uses the std::stoll (and similar) functions to parse the values which throw std::exception so i wouldn't change the catch there).
Another reason is that it's sometimes difficult to find out which NVP exactly is missing if you have nested data structures like:

{
  "id": 1,
  "config": {
     "id": 2,
     "data": {
         "identifier": 3
      }
   }
}

@AzothAmmo AzothAmmo added this to the v1.3.0 milestone Aug 24, 2016
@hoensr hoensr mentioned this pull request Apr 11, 2017
@wheeler1818
Copy link

any news here? a configurable root node would be helpful.

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

Successfully merging this pull request may close these issues.

3 participants