Non-consistent use of objects/pointers

In the whole package, it is not clear when pointers are used. There can be two string-type fields in one structure. One of them will be a pointer, and the second is not. Example: https://github.com/dstotijn/go-notion/blob/c675641a4c9f2c169b6b86b7fc8bb6adfb1b6a66/rich_text.go#L3-L12

Or a more illustrative example: https://github.com/dstotijn/go-notion/blob/c675641a4c9f2c169b6b86b7fc8bb6adfb1b6a66/page.go#L44-L67

Why is a Date field a pointer to a Date type, and a RichText a list of structures, not a list of pointers to them?

P.S. I'm not the most experienced go developer in the world 😀, so I may not understand a lot.

Hi Maksim, here's some context that will hopefully clear some things up:

There can be two string-type fields in one structure. One of them will be a pointer, and the second is not.

Some types (e.g. RichText) are used both for sending data to the API, as well as receiving. At the moment, the types are closely mapped to the data structures defined on the API (see the note a the bottom). When sending to the API, we want to omit fields that aren't explicitly set. With encoding/json, the empty value of a struct will not be omitted from JSON, but the empty value of a *struct will be omitted.

As for mixing string and *string, in RichText: you've got a valid point. Both plain_text and href are JSON object keys in the Notion API never need to be sent to the Notion API (they're computed values from the API, not settable/mutable), and thus there is no need to distinguish between the empty value of a string ("") and it being undefined (null). So, RichText.HRef should become a string. I'll do this in a later commit. I'll probably group these "read only" fields separately with a comment in the struct definition.

Why is a Date field a pointer to a Date type, and a RichText a list of structures, not a list of pointers to them?

See the previous note on why I've used pointers for fields that are used both for encoding to JSON as well as decoding from JSON.

As for slices, when using "omitempty", encoding/json will omit these from JSON when they're empty (ref). For our JSON encoding needs, there's no reason here to use []*RichText; whereas for *Date we do have a reason; because we want it to be omitted from JSON when marshalling.

Side note on the structure/define the types related to API requests and responses: To make things a bit more idiomatic, an alternative to DatabaseProperty being a somewhat generic struct with a bunch of pointer fields is to make it an interface. That way, there's less guess work involved when crafting requests to make to the API.

Here's how that interface could look:


type DatabaseProperty interface {
    ID() string
    Type() string
}

The various database property types would each implement this interface. And theUnmarshalJSON method on DatabaseProperties would be responsible for figuring out what type to decode the JSON into.

Curious to hear your thoughts on this "DatabaseProperty as an interface" approach.

Hi, I was actually in the process of writing a client myself when the API went out (dropped the project due to time), and encountered the same problem.

Side note on the structure/define the types related to API requests and responses: To make things a bit more idiomatic, an alternative to DatabaseProperty being a somewhat generic struct with a bunch of pointer fields is to make it an interface. That way, there's less guess work involved when crafting requests to make to the API.

Here's how that interface could look:
type DatabaseProperty interface {
  ID() string
  Type() string
}
The various database property types would each implement this interface. And theUnmarshalJSON method on DatabaseProperties would be responsible for figuring out what type to decode the JSON into.

Curious to hear your thoughts on this "DatabaseProperty as an interface" approach.

The best way in my opinion for an interface like this would be for it to implement the "Base type" where it returns all the properties present in all the blocks, but then all that's left to you is type assertion.

type DefaultBlock struct {
    Object         string     `json:"object"`
    ID             string     `json:"id,omitempty"`
    Type           BlockType  `json:"type"`
    CreatedTime    *time.Time `json:"created_time,omitempty"`
    LastEditedTime *time.Time `json:"last_edited_time,omitempty"`
    HasChildren    bool       `json:"has_children,omitempty"`
}

type Blocker interface {
    Block() DefaultBlock
}

type Heading struct {
    DefaultBlock
    Text []RichText `json:"text"`
}

func (h Heading) Block() DefaultBlock {
    return p.DefaultBlock
}

After messing around with it for a while, the API isn't any nicer with so much type assertion. It would be much better with proper union types, but as of right now, a massive struct of nullable fields is probably the best option

dstotijn / go-notion

Non-consistent use of objects/pointers #13