[WIP] Add unknown member to enums

[martinscholz83]

Fix #1502

[martinscholz83]

@ryangribble, waht do you mean with

add a BlahText property to always provide what the actual returned value was

[ryangribble]

What I meant by that comment was to have a string field containing the actual API response as well as the enum field containing the (attempted) deserialized enum value.

Eg say the field is public EventType Event, you would also have a field public string EventText

[ryangribble]

We also need to make sure to only do this for enums in response fields, we don't want to add this to enums only used on requests

[ryangribble]

We should use Enum.TryParse() method here, rather than deliberately throwing/catching an exception

[martinscholz83]

To use TryParse i need an instance TEnum. How can i achieve this from Type type

Enum.TryParse(stringValue, true, ????);

[ryangribble]

ah yeah sorry i didnt look at the diff context and see now that it's inside a non generic method etc, so the way you've done it is ok

[martinscholz83]

So, that means that we search all response types which have an enum, add a string field for that enum, and add a new member UnknownType for that enum. Right?

[ryangribble]

So, that means that we search all response types which have an enum, add a string field for that enum, and add a new member UnknownType for that enum. Right?

Yep that's what I was suggesting, although am interested in feedback from yourself and others, if this "feels" good...

[M-Zuber]

Feels good to me

[martinscholz83]

I think it's a good solution. My only concerns was that we miss if any new values are added by the api. So like @M-Zuber says, we should implement a mechanism which inform us if there is a new added member.

[M-Zuber]

That idea was only to allow us to then properly add the values into the enum, but I don't see a way to have octokit.net phone home from someones app, without them getting upset about it.

As long as we can safely parse any value by having a Unknown member to fallback on, that should be good enough for the medium term

[martinscholz83]

Ok, then lets go this way.

[ryangribble]

Sorry @maddin2016 I may not have been clear enough here. The changes you've made wont achieve the desired result if you consider this example using your method

public class SampleResponse
{
   public enum MyEnum { Value1, Value2, Unknown };

   public MyEnum? MyProperty { get; protected set; }

   public string MyPropertyText { get { return MyProperty.ToString(); }
}

And say the API response contains json { my_property: "value3" }

With your current changes we will get MyEnum.Unknown for MyProperty and "Unknown" for MyPropertyText but what we actually want to get is the unrecognized string "value3" for MyPropertyText.

There is also the case that when the field is null you will throw an exception trying to call .ToString() on it.

So what we actually need to do is something similar to the way we sometimes handle unix timestamps where we map the actual api response field (my_property) to the string field (MyPropertyText), and then have the enum field just have a get only property that attempts the enum parse (falling back to .Unknown if it couldn't parse and returning null if the string value was null).

something like this:

    public enum MyEnum
    {
        Value1,
        Value2,
        Unknown
    }

    public class SampleResponse
    {
        [Parameter(Key = "ignoreThisField")]
        public MyEnum? MyProperty { get { return MyPropertyText.ParseEnumWithDefault<MyEnum>(MyEnum.Unknown); } }

        [Parameter(Key = "my_property")]
        public string MyPropertyText { get; protected set; }
    }

With the helper function ParseEnumWithDefaut<TEnum>() being added to the StringExtensions class

StringExtensions.cs

        public static TEnum? ParseEnumWithDefault<TEnum>(this string input, TEnum defaultValue) where TEnum : struct
        {
            if (input == null) return null;

            TEnum value;
            return Enum.TryParse(input, out value) ? value : defaultValue;
        }

Thoughts?

[martinscholz83]

Ok. Now it's clear to me. I already had this in mind, but i don't know how to implement. Many thanks!!

[ryangribble]

Cool... A couple more thoughts:

• We shouldnt change the signature of the ctor's as they are public and it would be a breaking change. I guess we can still take the enum field in the ctor then assign it to the xxxxText property like StatusText = status.ToString()

• I guess the helper function doing the enum parsing needs to handle the transition from snake case to camel case too, like the JsonSerializer does. Eg the text value might be "client_interface" which we need to parse into xxxx.ClientInterface so it will need to convert snake_case to CamelCase first before trying to parse the value?

• Need to think about how to test we have these changes correct. We already have some serialization unit tests for response classes but I'd be suggesting that for each response class we touched, we add unit tests where we feed a json string containing a valid response and one containing a valid response EXCEPT the enum value is one that doesnt match the enum, and then assert all fields on the returned objects are as expected

• Will there be any performance impact? Previously the deserializer was caching enum deserializations so we were basically only "figuring out" each enum once no matter how many objects of that response type were encountered. Now we dont have any of that overhead but everytime the enum field is accessed we are parsing the string to an enum. If this does cause performance impact I guess we could set the enum field's value whenever we set the text value (ie the ctor and the setter of the text property) rather than everytime we access it. It may not even affect performance though

Im not sure if these changes are getting to kludgy though, need some feedback from @shiftkey if we are heading too far down a rabbit hole. I guess the alternative was throwing exceptions whenever an API response changed which wasnt great either :)

[martinscholz83]

Ok, then i wait for feedback from @shiftkey before going on.

[martinscholz83]

And what we should discuss is the naiming of the new member Unknown. Here enum member is called unknown. I know this not an enum used by any response, but what if they implement such an member by the api.

[andrejo-msft]

There is always the trouble of a partner reusing a synthetic value (ie: 'unknown'). unknown is probably sufficient, but more descriptive naming may lessen the risk.

An additional positive of more descriptive naming is that an external developer seeing the event type will know that this is an artifact of the octokit library as opposed to something odd with the REST api. octokit_unsupported_type or simply octokit_unsupported might be an alternative.

That said, thank you, @maddin2016. This will be a useful feature for our team.

[andrejo-msft]

Hey, folks. Any more thought on this PR? We are seeing more issues of the kind noted above in the original issue.

[ryangribble]

Bump @shiftkey if you want to comment at all?

Im happy to proceed with this change... My most recent points for consideration still stand.

And a couple of further points:

• I tend to agree with the comments about naming the placeholder value something more obvious... such as OctokitUnsupportedValue rather than just Unknown

• Continuing the second guessing over "Naming things", Im wondering if xxxRaw is better than xxxText for the string property that will hold the raw actual response. Thoughts?

• Since going down this road will impose the requirement on all future changes to handle enums on response classes in this way I would like us to "enforce" this by adding to our convention tests

  • find all enums that are used in Response classes and ensure they have this UnknownApiValue placeholder value
  • find all Response classes that have an enum typed member (eg Blah) and ensure they have a string BlahText property

[M-Zuber]

xxxRaw sounds better to me as well

[shiftkey]

@ryangribble @andrejo-msft @M-Zuber this all seems reasonable 👍

[khellang]

Is this change still needed after #1595?

[ryangribble]

Nope, this one can be closed in favour of #1595 👍