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

Stricter json error enforcement #33931

Merged
merged 3 commits into from Sep 10, 2019

Conversation

@jbytheway
Copy link
Contributor

commented Sep 10, 2019

Summary

SUMMARY: Infrastructure "Stricter json parsing; errors will occur in more places. This may cause some mods to fail to load"

Purpose of change

Our json parsing code is very permissive, and quite a lot of errors can go unnoticed. It will help content designers if we detect more errors, especially if they're detected in CI.

Describe the solution

Be stricter about parsing in the following way:

Add a bool throw_on_error to all JsonIn::read functions. If true, this causes them to throw an exception on error rather than just returning false. The exception is thrown via the usual JsonIn::error function that provides a useful error message.

This bool defaults to false, but defaults to true for calls via JsonObject::read, which constitute the bulk of our json reading of game data files. Those functions also gained a new parameter so you can override back to non-throwing if you wish.

This caught some problems, which I've fixed in this PR:

  • Prices should be integers, not strings.
  • NPC dialogue opinion changes should be integers, not arrays.
  • refuel_fires should be a boolean, not a string.
  • damage_instance needs to be able to load from a json array as well as a json object.
  • When a read call fails for a JsonObject member, give a different error message depending on whether any member with that name was present.

Describe alternatives you've considered

I had to choose new values for the dialogue opinion changes. I assumed that they were intended to be random selections from a range, so I took the midpoint of the range, and rounded to the nearest integer (rounding in the direction more favourable to the player). Then removed the ones that were just zero after that. @I-am-Erk might want to make further changes depending on the original intention.

Additional context

I want to further increase json parsing strictness. This is just the first step.

I tested with many of the included mods, but not all of them, so this might break some of the core mods, and it could certainly break third party mods. But usually the fix is pretty obvious.

Stricter json error enforcement
Throw errors in more cases for json read problems.

To catch errors sooner when there are typos or similar issues in the
game data.
ZhilkinSerg added 2 commits Sep 10, 2019

@ZhilkinSerg ZhilkinSerg merged commit aaf90dc into CleverRaven:master Sep 10, 2019

3 of 5 checks passed

Build
Details
JSON style check
Details
Validate PR
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@ShadySand

This comment has been minimized.

Copy link

commented Sep 10, 2019

CRT_Expansion mod not loading with newest version, its one of the default mods, Expected string 14:22 in \data\mods\CRT_EXPANSION\crt_materials.json

@kevingranade

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/problem-launching-a-new-game/21303/2

@throttlekitty

This comment has been minimized.

@jbytheway jbytheway deleted the jbytheway:stricter_json_parsing branch Sep 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.