Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generated models can be difficult to use due to unnecessary pointer types #19

Closed
nmiodice opened this issue Sep 3, 2019 · 4 comments
Closed
Assignees

Comments

@nmiodice
Copy link

nmiodice commented Sep 3, 2019

The generated models can be difficult to use with primitive types like string, int and bool because they are consumed as pointers and can make the code more verbose.

For example, in order to use this struct with hard-coded values, I need to write my code like so:

abbreviation := "..."
description := "..."
version := 1

// note the use of variables. in Go, you cannot de-reference a constant!
project := core.ProjectInfo{
	Abbreviation: &abbreviation
	Description: &description 
	Version: &version
        ...
}

Without pointers, it can be simplified to this:

project := core.ProjectInfo{
	Abbreviation: "..."
	Description: "..." 
	Version: 1
        ...
}

Note: the current strategy probably lowers the runtime memory footprint of an application for larger types like strings, structs, maps, etc... but the efficiency gain is lost for int and bool (i believe).

@nmiodice nmiodice changed the title Generated models have difficult to use primitive types Generated models can be difficult to use based on unnecessary pointer types Sep 3, 2019
@nmiodice nmiodice changed the title Generated models can be difficult to use based on unnecessary pointer types Generated models can be difficult to use due to unnecessary pointer types Sep 3, 2019
@tedchamb
Copy link
Member

tedchamb commented Sep 4, 2019

Thanks for the feedback. A lot of thought was given in to how to handle the generated models, although that doesn't mean I got it right. I agree that the verbosity of creating a new struct with hard coded values is not great.

Let me walk through the thinking behind the current approach. The service is written in c#, so we need to be able to represent all the data in Go that can be represented in c#. Since structs in Go can not be null, we are forced to use a pointer for struct fields, so that the nil pointer can represent that null state.

string is a similar case. In c# the empty string and a null string have two different meanings. For example, if I make a patch request with a description field set to the empty string, this will update the resource's description to an empty string, while making that same patch request with the description field set to null (or not supplied), will leave the description unchanged. In Go the only way to represent a null string, is to have a nil string pointer. Therefore it was necessary to make string fields pointers also, so we can cover that null case.

Simple types like int and bool are handled similarly in both languages, and therefore we could get away with making these non pointer fields in Go. What you will find throughout our c# models is that for many of the cases the nullable types int? and bool? are used rather than simply int or bool. Patch requests are probably the biggest reason for this. For int? and bool? we will still have to use pointers to be able to represent the null state. For our c# controller methods, all new parameters that are added have to be optional for compatibility reasons, so simple types that are query parameters are usually nullable also. This means that our parameter structs in Go will also require the pointers for most cases of the simple types.

This leaves us with the decision on how to handle the simple types that are not nullable:

Option 1: Keep all simple type fields as pointers. This has the drawback of extra verbosity. The benefits are in the consistency. The consumer does not have to be confused at why some int fields are pointers and others are not. They know ahead of time that the field is a pointer because every field is a pointer.

Option 2: Do not use pointers for simple types that are not nullable. This removes a small amount of the verbosity, making it very easy to set values on those fields. The drawback is that this only helps in a subset of fields, and introduces the inconsistency mentioned above.

What are your thoughts? Of the two options, which would have been better for your project?

@tedchamb
Copy link
Member

@nmiodice , have you had a chance to review my response above? Do you have any feedback regarding the response?

@mcdafydd
Copy link

I'm glad to see a complete Azure Devops library for go coming to fruition, thanks for your effort!

I've been following the current go-github approaches, which is to use pointers for all struct members, along with generated accessors and helper functions like https://github.com/google/go-github/blob/master/github/github.go#L1042-L1056. They discussed this topic in several issues over the years. There are some interesting comments about taking a different approach if they started over, like google/go-github#537 (comment), but they stuck with pointers for consistency.

From a consumer and new amateur Go user standpoint, I don't mind the extra verbosity too much. I appreciate the readability and don't mind checking for nil all over the place.

@tedchamb tedchamb self-assigned this Oct 4, 2019
@tedchamb
Copy link
Member

Thanks for the info/feedback @mcdafydd. Based on the information you provided, I think it makes the most sense to keep things as they are. I am closing this issue for now, and we can reopen if a compelling alternative pops up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants