Page MenuHomePhabricator

C# documentation for tuple properties is wrong
Closed, ResolvedPublic

Description

Tuple-valued properties do not have their value documented properly. It looks like only the docs from the first item in the tuple are used.

I am no expert in C# but found this in StackOverflow:
https://stackoverflow.com/a/8962062

Looks like we should not be using tuples as return values after all. It is recommended to use a explicit class, with proper fields and docs.

segfaultxavi triaged this task as High priority.

We should do something about this, docs are incomplete right now.

jptiz added a comment.EditedDec 12 2019, 3:07 PM

Looks like we should not be using tuples as return values after all. It is recommended to use a explicit class, with proper fields and docs.

I agree that returning explicit types looks more realistic and "C#-ish" than tuples, but how would it work?

For now, what I can think is something like this .eo:

@property composite_colour {
    set {
        [[Sets a composite RGB colour.]]
    }
    get {
        [[Gets the composite RGB colour.]]
    }
    values {
        r: int; [[The red component.]]
        g: int; [[The green component.]]
        b: int; [[The blue component.]]
    }
}

Would generate a CompositeColourValue struct, like this:

/// <docs>
public struct CompositeColourValue {
    int R;
    int G;
    int B;
}

/// <docs>
public CompositeColourValue CompositeColour { get; set; }

But I'm not sure that would be the expected behaviour (neither I know if it wouldn't be adding unnecessary extra complexity).

That's what I was thinking of, yes. And I also agree it looks unnecessarily complicated.

At the very least, we can document the tuple content:

///<summary>A tuple property.</summary>
///<value>A tuple containing the following information:
///<list type="bullet">
///<item><description><c>a</c>: Parameter a.</description></item>
///<item><description><c>b</c>: Parameter b.</description></item>
///</list></value>
public (int a, int b) tupleproperty;

I have just checked and DocFX renders the above code correctly.

felipealmeida added a subscriber: lauromoura.

Creating a new class for each tuple return does look more complicated than it needs to be IMO. However, with indexer, we are already creating new classes anyway, so maybe it could be worth it?

I'm OK either way as long as everything is properly documented. Tuples+D10889 looked OK but I haven't looked at the current API using indexers yet.

felipealmeida closed this task as Resolved.Feb 13 2020, 10:39 AM