Detect unused json members #34735
Detect unused json members #34735
Conversation
KorGgenT
added
[JSON]
| @@ -893,7 +893,6 @@ | |||
| "autolearn": false, | |||
| "never_learn": true, | |||
| "time": "510 m", | |||
| "blueprint_requires": [ { "id": "not_an_upgrade" } ], | |||
There was a problem hiding this comment.
This seems intended: #34661 (comment)
There was a problem hiding this comment.
please don't take these out again. It is needed to keep the blacksmith survey suppressed from the bulletin board menu per @mlangsdorf
There was a problem hiding this comment.
OK, will look into it more.
There was a problem hiding this comment.
This isn't removing it from the blacksmith entry, only from the faction wall entries, which I believe to be correct.
There was a problem hiding this comment.
I agree that it's not strictly needed for the faction wall entries, but I'd like to keep it anyway because it makes it clearer that they shouldn't go in the general upgrade missions.
There was a problem hiding this comment.
Well, I can't leave things how they are, because the tests will fail under this new regime. If we want to keep them then I suggest either:
- Make it a 'commented out' field by renaming it
"//blueprint_requires"or - Tweak the loading code in
recipe.cppso that it visits theblueprint_requiresfield even when it ignores it.
I'm leaning towards the first option. Any preference?
There was a problem hiding this comment.
So, what's the status quo here?
There was a problem hiding this comment.
I went for the 'commented out' option. @mlangsdorf welcome to tweak as desired now that this is merged.
jbytheway
force-pushed
the
detect_unsed_json_members
branch
from
ef68625 to
7cfc274
Compare
jbytheway
changed the title
jbytheway
force-pushed
the
detect_unsed_json_members
branch
3 times, most recently
from
b926d2b to
51eef01
Compare
In response to discussion on CleverRaven#34735
jbytheway
changed the title
Verify when we destroy a JsonObject that we actually visited all the fields of that object. If we didn't, this suggests an error of some kind in the input data. Default is to report an error to the debug log, which users won't notice, but will cause a test failure in CI. This means we need to nearly always pass JsonObjects by non-const reference, so that all the member visitations get registered in a single place. One can opt out of this check by calling allow_omitted_members on the JsonObject. We need to add such calls in a bunch of places that don't visit all the members for one reason or another. Also added various other tweaks and workarounds to prevent false error report. The checking has to not be turned on when building the json tooling executables, because it leads to linking errors (the debug log doesn't exist for them).
These are a collection of new json fixes newly required since I first made this PR.
In response to discussion on CleverRaven#34735
jbytheway
force-pushed
the
detect_unsed_json_members
branch
from
a206df2 to
0ec0c52
Compare
|
I've resolved the above-discussed issue by the "commenting out" solution, and I think I've finally managed to catch up and fix the errors other people are introducing fast enough to get a clean test build, so this should be ready to merge now. |
Summary
SUMMARY: Infrastructure "Detect unsed json object members when parsing json data"
Purpose of change
Our json data formats are quite permissive in that many members are optional. Consequently, it's easy to make a mistake in the json which goes unnoticed. For example a typo in a key name, or a misplaced bracket, or a simple misunderstanding of the format.
This happens a lot. See #33144 #33328 #33502 #33613 #33852 for examples of fixes of this type. I'd like to catch them earlier.
Describe the solution
Each
JsonObjecttracks every member which is visitied. When it is destroyed, it logs an error if not all the members were visited.This error will not show up for the user (it's not a
debugmsg) but it will cause the tests to fail.It's possible to opt out of this on a per-
JsonObjectbasis by callingallow_omitted_members().Calling
str()will also suppress the errors.I fixed most of the errors that arise in the previous PRs #34455 and #34525. Those only cover the core data and Magiclysm. Some more problems have been added since I did so, and those are fixed here. Hopefully that's enough for the CI tests to pass. If you play with other mods the errors will spam the log somewhat, but I wanted to get this into
masterto prevent further growth of more json issues in the core game data.Describe alternatives you've considered
We could try to have some sort of json schema, but maintaining it in parallel with the parsing code seems like a recipe for inconsistency.
Testing
The code itself is hopefully fairly safe. At worst it will log some inaccurate errors. But I'm fairly sure I've correctly interpreted the situations that arise in the core game data and have suppressed the errors in the code as appropriate.
Mostly relying on the unit tests and a brief launch of the game to ensure nothing is unexpectedly broken.
Additional context
The obvious next step is to fix these errors in all the other in-repo mods. I'm not sure whether I will get around to that.