feat: Add canonical serialization mode to event and variable serialization#8552
feat: Add canonical serialization mode to event and variable serialization#8552ViktorVovk wants to merge 1 commit into
Conversation
- Introduced a global canonical mode in the Serializer class to ensure consistent JSON output. - Updated various event serialization methods to include default values and alphabetical key ordering when canonical mode is enabled. - Added support for canonical serialization in the Variable class. - Enhanced the SerializerElement to handle canonical mode during JSON conversion. - Updated related tests and preferences to accommodate the new serialization behavior.
Shouldn't this make things harder to review for a human (or even an AI) because there is less noise?
Agree it's not great and probably something we can fix! |
|
Good day @4ian , and thank you for the preliminary review. I’ll try to answer your question. The idea is that when the This way, when a value changes, only the value itself changes, making the diff much clearer — you can simply see that a property changed from With the default behavior, when a value is |
|
Ok, I see the reason behind this. We will see if we can make the implementation a bit less verbose with some dedicated helper (SetBoolAttributeOrEmpty? something that conveys that if "canonical" is not activated, a falsy or default value is not set). Then this will be good to go! |
| for (const auto& entry : sortedEntries) { | ||
| Value name(entry.first.c_str(), allocator); | ||
| Value childValue; | ||
| if (entry.second.isAttribute) { |
There was a problem hiding this comment.
Nitpicking: remove the boolean entirely and instead relying on checking if entry.second.attributeValue != nullptr
(and same for entry.second.childElement != nullptr, out of safety)
| private: | ||
| Serializer(){}; | ||
|
|
||
| static bool s_canonicalMode; |
There was a problem hiding this comment.
We usually don't use prefix with hungarian notation:
| static bool s_canonicalMode; | |
| static bool canonicalMode; |
4ian
left a comment
There was a problem hiding this comment.
Added a few comments and I think it will be fine to have this.
I would want ideally all SerializeTo methods to handle silently canonical, but I don't have yet a good idea of how to make this elegantly.
Add
canonicalEventSerializationpreference for stable, diff-friendly event JSONProblem
GDevelop's event serialization omits properties that hold default values —
disabled,folded,inverted,awaitare only written to JSON whentrue;events,variables,subInstructions,loopIndexVariable, etc. are only written when non-empty. This causes two kinds of noise in git diffs:Both issues make per-event diffs hard to review in version-controlled projects.
Solution
Add a project-level preference
canonicalEventSerialization(opt-in, defaultfalse). When enabled:false,"",[]), so toggling a flag produces a single-line value change, not an insertion/deletion.The preference is controlled via
gdevelop-settings.yaml(already read by the IDE on project open):