Skip to content

Standardize response model property accessors #2472

@nickfloyd

Description

@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

Metadata

Metadata

Assignees

Labels

Status: Up for grabsIssues that are ready to be worked on by anyoneType: MaintenanceAny dependency, housekeeping, and clean up Issue or PR

Type

No type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions