Fix handling of empty values on documentation generation #20250
Conversation
…y string to null No value makes sense if the field has FieldLoader.RequireAttribute, even an empty value. For the sake of documentation generation best use null consistently.
FieldSaver.SaveField() would always produce an empty string if the value is either an empty string or null, which isn't great for documentation generation. Add special-case handling of those empty values so the final result reflects the actual field value. Keep an empty string in the Markdown documentation to maintain the previous behaviour there.
| @@ -26,7 +26,7 @@ public class ClonesProducedUnitsInfo : ConditionalTraitInfo, Requires<Production | |||
|
|
|||
| [FieldLoader.Require] | |||
| [Desc("e.g. Infantry, Vehicles, Aircraft, Buildings")] | |||
| public readonly string ProductionType = ""; | |||
| public readonly string ProductionType = null; | |||
There was a problem hiding this comment.
| public readonly string ProductionType = null; | |
| public readonly string ProductionType; |
Couldn't we just do this instead? It's null by default
| @@ -83,7 +83,7 @@ def format_docs(version, collectionName, types): | |||
|
|
|||
| print(f'| {prop["PropertyName"]} | {defaultValue} | {prop["UserFriendlyType"]} | {prop["Description"]} |') | |||
| else: | |||
| print(f'| {prop["PropertyName"]} | {prop["DefaultValue"]} | {prop["UserFriendlyType"]} | {prop["Description"]} |') | |||
| print(f'| {prop["PropertyName"]} | {prop["DefaultValue"] or ""} | {prop["UserFriendlyType"]} | {prop["Description"]} |') | |||
There was a problem hiding this comment.
Why is this needed? When I revert this change the end result is identical
There was a problem hiding this comment.
When you actually have null values in the generated JSON (which happens for sequences), the Python script serializes that as "None", hence this check to not show anything in the Markdown.
|
|
||
| return new | ||
| // HACK: FieldSaver always produces an empty string if there is no value, but it can be important to distinguish between "" and null. |
There was a problem hiding this comment.
Would adding a replaceNullWithEmptyString: false argument to FieldSaver.SaveField solve the disagreements over this?
There was a problem hiding this comment.
Something like that could probably work, yes, or perhaps going directly for FieldSaver.FormatValue(object v) instead of FieldSaver.SaveField().
|
Is there a difference for the end user between an empty string an an uninitiated string? If not, then we don't need to document it. If there indeed isn't a difference we could instead choose one and apply a code style rule to all strings, forcing the use of either empty or uninitialised strings. |
|
I don't see the need for the question here as I already tried explaining multiple times on Discord and was ignored (I understand that this is a simple PR bump). People just decided they don't care about a difference there. So I'm just going to drop the changes here and move on to the next PR. |
The problem was that you didn't mention a single difference, the question was never answered! |
|
I think it's reasonable to ask for usecases / explanation before introducing a HACK and from it judge if a better solution could be used instead. Instaed of an answer you basically just said." it's better, do you not trust me?" |
|
And then just started ignoring the subject until I posted it on github |
I noticed something we missed in #19948 - we never had
nullas a default value for a trait or a weapon field. The reason being howFieldSaver.SaveField()works.Since the distinction between an empty string and
nullcan be important for consumers of the documentation, I added a quick "hack" to fix it. Also no values, including empty strings, make sense on fields withFieldLoader.RequireAttribute, so I changed those tonull. No update rules needed either.I suggest reviewing with whitespace changes hidden - https://github.com/OpenRA/OpenRA/pull/20250/files?w=1