Page MenuHomePhabricator

document default values of properties
Open, HighPublic

Description

I think all properties in eo files should be required to specify what their default values are. This is especially important for interfaces to ensure that all implementations use the same default values.

zmike created this task.Aug 22 2019, 5:23 AM
zmike triaged this task as High priority.

Hmmmm.... agreed.

Doc generators can then read these default values and print them in a unified way instead of relying on this information being present in the text.
@q66, In this case:

@property something {
   values {
      val : bool(false);
   }
}

The default value can be retrieved with eolian_function_return_default_value_get(), right?

Also, @q66, can we add the traditional EOLIAN_ envvar check that errors out on missing default values?

Finally, these default values are purely informational and that's a pity. The implementation could change and then the eo file would be out-of-sync and nobody would notice. Is there any way to ensure that they are indeed the default values? In the test suite, for example? @zmike, @bu5hm4n, @cedric?

@zmike @bu5hm4n Do you think it is possible to create unit tests that check that default values are being honored by all implementations? Should we create a task for that?

What is the point of the default values ? They are *only* returned when the object is NULL or invalid, and the plan for that was to Error out anyways, so we might can just default to 0 (in the sense of the type) for that.
I must admit that i do not see much value in this task here...

zmike added a comment.Sep 25 2019, 9:12 AM

No, there's also the case where an error occurs after object validation inside the body of the function.

My idea was that we could use also the eolian default values as actual property default values. This has no effect on implementations, they must just make sure they obey this default value, but its nice for documentation, because then we can remove all the "The default value is ..." from docs, since it will be autogenerated (This is already done and landed, btw).
But we need to make sure classes DO obey these default values, and that's why I'm asking for a unit test.

This has been sitting here for a month and we need to do something about it, because the current state is not ideal:
The C# generator automatically adds docs to properties saying "The default value is X", taking that value from the the EO file.
But the truth is that implementations are free to use any default value they please, regardless of what the EO file says.
Default values currently in the EO files were chosen to match the implementations, but it is almost guaranteed that they will fall out of sync in the future.

My proposal was to ENFORCE (via unit tests) that implementations do comply with the default values. Any other proposal?