Standardize response model property accessors

[nickfloyd]

As we start making our way toward generating models based on the OpenAPI definitions from the GitHub REST API we need to do some pre-work to help us know what the result of the generated code might look like. This begins with standardization, establishing patterns, and approaches to the way we build our models.

One area where we need to do this is with our response models. Given that most of them have constructors now we should be able to move to a standardized way of writing these models. Considering the following:

• Some models use protected, private, or no modifier on the setters
• Some models are missing constructors
• Some models implement DebuggerDisplay property that has been implemented in a few different ways.

I recommend we standardize on the following.

• We still use DebuggerDisplay
• There should be two public constructors one that is empty and the other setting all the properties from parameters being passed in; this allows the construction of an empty object as well as providing a construction contract.
• Setters will be made private favoring a more restrictive pattern - this clearly communicates the intent of the model itself - that it is only ever hydrated from a source once and should never mutate (that's what the request objects are for) as well as indicate that the memorized object map is the primary definition of how a response model gets hydrated.
• The DebuggerDisplay property can be simplified using (assuming a non static model) - get { return JsonSerializer.Serialize(this);}. This eliminates the need for having to update it every time properties are introduced, will provide the same shape as the actual model, and provides a serialized version during debugging that can be used for various dev needs.

example:

using System.Text.Json;
using System.Diagnostics;

namespace Octokit
{
    [DebuggerDisplay("{DebuggerDisplay,nq}")]
    public class SourceInfo
    {
        public SourceInfo() { }

        public SourceInfo(User actor, int id, Issue issue, string url)
        {
            Actor = actor;
            Id = id;
            Issue = issue;
            Url = url;
        }

        public User Actor { get; private set; }
        public int Id { get; private set; }
        public Issue Issue { get; private set; }
        public string Url { get; private set; }

        internal string DebuggerDisplay
        {
            get { return JsonSerializer.Serialize(this);}
        }
    }
}

Hat tip to @janv8000 for the more clean/restricted thoughts on setters in his PR

[timrogers]

This sounds good to me 🏁 I'm very in favour of standardisation.

[JonruAlveus]

Which JsonSerializer are you referring to? I can't see any static serializers currently available in the repo (and system.text.json requires .net 4.6.1).

[JonruAlveus]

Changing this test to throw when models have non private setters (or none) reveals... only a small amount of work!

[nickfloyd]

Which JsonSerializer are you referring to? I can't see any static serializers currently available in the repo (and system.text.json requires .net 4.6.1).

That's a great point on the requirement around 4.6.1 - it seems it would be best to revert back to string interpolation. I will roll through a few thoughts on how to simplify things here (though it's a low priority). Also, this may be a needless optimization (in terms of code writing) as we get closer to generating these models based on the OpenAPI descriptions.

[JonruAlveus]

We could use the SimpleJsonSerializer (or SimpleJson directly). With the auto generated api descriptions, what's the future of that serializer?

What does the path look like to using OpenApi descriptions and would it be worth having a discussion (separately to this thread) about a 4.6.1 retargetting?