Unecessary Pointers

[mavolin]

Unecessary Pointers

[diamondburned]

-1 null Snowflake added in 55323ee

[mavolin]

Nah I'm just dumb. Discord normally uses a question mark for optional fields, but if all fields are optional they just write a comment and don't bother to add the question mark.

[diamondburned]

Go's json actually treats both a nil slice and a zero-length slice as empty, so omitempty would omit that.

Sometimes, a nullable and optional snowflake is also needed. Having it as a pointer solves this.

[diamondburned]

Here's an example. Take UpdateStatusData's Activities field.

This field is required to either not be present, meaning it wouldn't be updated, or it has to be an empty slice, clearing out the user status, or a slice with Activity in it, setting a new status.

This playground link should demonstrate that a []T wouldn't work in this case: https://play.golang.org/p/lljPmhfgvL6.

type T struct {
	Slice1 []string  `json:"slice1,omitempty"` // non-nil slice
	Slice3 *[]string `json:"slice3,omitempty"` // non-nil pointer and slice
}

The above structure, when marshaled with a length 0 slice, would give the following JSON:

{"slice3":[]}

As shown, the zero-length slice does not get marshalled into an empty slice and instead gets omitted, while the pointer slice gets marshalled as expected.

[mavolin]

Go's json actually treats both a nil slice and a zero-length slice as empty, so omitempty would omit that.

But afaik there is no case in which an empty slice would cause a different behavior than an omitted slice.

Sometimes, a nullable and optional snowflake is also needed. Having it as a pointer solves this.

Of course, but seeing as there neither are and nor will be any Snowflakes with the value 0 (the timestamp alone would prevent that), a Snowflake with its zero value would fully suffice and reduce the amount of extra work and inelegance by this solution. Or am I missing a case where this does not apply?

[diamondburned]

A zero snowflake actually marshals into null:

func (s *Snowflake) MarshalJSON() ([]byte, error) {
	var id string

	switch i := int64(*s); i {
	case -1: // @me
		id = "@me"
	case 0:
		return []byte("null"), nil
	default:
		id = strconv.FormatInt(i, 10)
	}

	return []byte(`"` + id + `"`), nil
}

(Sidenote: the -1 API should probably be deprecated.)

Edit: As far as I know, MarshalJSON() does not control whether or not the JSON field will be omitted. It seems like Go's json will just read the underlying value using reflect and determine that instead of scanning the output.

[mavolin]

Ok, I see, and I'm starting to hate go for this. Sorry for bothering you with the other stuff, I should probably check the docs before I create an issue complaining about something. ^^

But in the interest of keeping it simple, (nil = omit and 0 = null), wouldn't it make more sense to use 0 to omit the Snowflake and create a var SnowflakeNull Snowflake = 1 and include it in the MarshalJSON, seeing as 1 will also never be used
This would make explicitly writing &0 for null, which is kinda contrary, redundant.

[diamondburned]

Changing 0 to 1/-1 for null would probably break a lot of things. I think it's a good idea too, buuuttt...

[diamondburned]

On second thought, we could probably make anything less than 1 be null. -1 wouldn't count as empty, thus would be kept.

// NullSnowflake gets encoded into a null. This is used for
// optional and nullable snowflake fields.
const NullSnowflake Snowflake = -1

//      v No longer a pointer
func (s Snowflake) MarshalJSON() ([]byte, error) {
	if s < 1 {
		return []byte("null"), nil
	} else {
		return []byte(`"` + strconv.FormatInt(i, 10) + `"`), nil
	}
}

[diamondburned]

On a side note, I discovered that the ModifyGuildData.Name shouldn't have the omitempty tag. While it won't make any difference as the discord API will return an error anyways, it still help with clarity.

Discord documented Modify Guild as having all fields being optional, so I'm not sure what's wrong here.

Maybe you were confusing it with CreateGuildData?

[mavolin]

Could the NullT constant syntax be extended to the types ExplicitFilter, Notification, and Verification, as they still require the complicated usage below?

var f ExplicitFilter = 1

SomeStructUsingThis {
    ExplicitFilter = &f
}

If you approve, I'm happy to hand in a PR implementing the changes.

[diamondburned]

Yeah, sounds good to me. You should also extend it to all positive-only integer types such as Milliseconds, if possible.

[mavolin]

Considering there are multiple custom types, many of which share the same underlying types, I'd suggest creating a type that implements the json.Marshaller and json.Unmarshaller interfaces and provides the nullable option.
Does a nullable package, with types like nullable.Int8 etc. in utils sound good to you?

[diamondburned]

That should be json, though I've been meaning to make a json/option package, so sure, that'll do. Call it option though, since current optional types are already json.OptionT.

[mavolin]

But wouldn't the type's namespace collide, seeing as we already have types like json.OptionInt? Also, as OptionT types work quite differently (pointers to types with dedicated functions to create those types) compared to the types with a NullT constant, that is checked for during (un-) marshaling, so they need another name or package.

Btw I can refactor the current option types and put them into an option package, shouldn't be to time-consuming, and matches the topic of the PR.

[diamondburned]

It would be moving OptionInt and such into package option, so I don't see how they would collide. Those default OptionT that are pointers to types can be kept, but you could also make an OptionalPosInt type (I'm terrible at naming) that would use a NullT.

Basically, keep OptionT's implementations abstracted, so NullT wouldn't make a lot of difference (other than replacing nil with NullT, if you know what I mean). You can go ahead and do the PR.

P/S: sorry for the late response, I was doing #24 and forgot about this one :(

[mavolin]

Maybe I misunderstood, but wouldn't types like option.Uint and option.PosUint be a bit hard to differentiate, as the types with NullT are omittable and nullable and their underlying type is typically never interacted with, as there are used for enums, in comparison to the option.T types which are just omittable and are directly interacted with, as their values are not predefined. Or did you actually mean that they should be separated in a json/option package and a json/nullable package?

Also, I forgot to mention, that this change would require that the types affected to don't store their default as constants, but as variables so that all in all this works:

package utils/json/nullable

// Null is the value used to represent JSON null.
// It should never be used as a value, as it won't
// get serialized as such.
const Int8Null = -1

type Int8 int8

func Int8ToJSON(i Int8) []byte {
	if i == Int8Null {
		return []byte("null")
	} else {
		return []byte(strconv.Itoa(int(i)))
	}
}

func Int8FromJSON(b []byte) (Int8, error) {
	s := string(b)

	if s == "null" {
		return Int8Null, nil
	} else {
		i, err := strconv.ParseInt(s, 10, 8)
		return Int8(i), err
	}
}

package discord

type Verification nullable.Int8

var (
	// NullVerification marshals to JSON null and may only be used for types
	// that are nullable.
	NullVerification Verification = Verification(nullable.Int8Null) // -1
	NoVerification Verification = 0
	// LowVerification requires a verified email
	LowVerification Verification = 1
	// MediumVerification requires the user be registered for at least 5
	// minutes.
	MediumVerification Verification = 2
	// HighVerification requires the member be in the server for more than 10
	// minutes.
	HighVerification Verification = 3
	// VeryHighVerification requires the member to have a verified phone
	// number.
	VeryHighVerification Verification = 4
)

// Go doesn't allow to inherit UnmarshalJSON and MarshalJSON from Int8, so we have to
// manually implement them.
// And since we can't convert a pointer of one type to a pointer of another type, we
// might as well just use Int8FromJSON and Int8ToJSON instead of having to do multiple
// type conversions.
func (v *Verification) UnmarshalJSON(b []byte) error {
	i, err := Int8FromJSON(b)
	*v = Verification(i)

	return err
}

func (v Verification) MarshalJSON() ([]byte, error) { return Int8ToJSON(Int8(v)), nil }

api.ModifyGuildData{
    Verification: &NullVerification, // type *Verification
}

I think the var option is actually the one with the smallest tradeoff, seeing as otherwise syntax like this will be required: Verification: *Verification(nullable.Int8(NullVerification))

Btw, no worries with replying late, we are in different time zones (I'm from Germany, so -6 h), and it was nearly 4 am when I last replied, so I would have continued the conversation later anyways.

[diamondburned]

I think something like the Verification constant could be a type alias to avoid boilerplate when using NullInt8: https://play.golang.org/p/lvgybZ--2Se.

Also, I agree on making an option and null package, though it seems to me those two behave pretty much the same with the difference being the JSON tags, not the actual implementation of their types. Maybe they could be made the same?

[mavolin]

First of all we still need a bitsize of 8, according to the docs:

The bitSize argument specifies the integer type that the result must fit into. Bit sizes 0, 8, 16, 32, and 64 correspond to int, int8, int16, int32, and int64. If bitSize is below 0 or above 64, an error is returned.

You used ParseUint, not ParseInt, so you are right, I overread that.

Secondly, I don't see how we could make them the same. You can have a look at my fork, I've just pushed the changes for Verification and refactored the option types and put them into the option package.

Regarding the two types:

option.T

• A pointerized by default, as they are only used on optional fields
• Unknown values, must be user generated and therefore direct interaction with the package is required
• Not Nullable, just omittable

nullable.T

• Not pointerized by default, as they are used for enums that are sometimes omitted and sometimes required
• Known values, as they are enum types and therefore aren't interacted with directly, but rather through wrapper types
• Nullable, and sometimes omittable

[diamondburned]

That's weird, because the playground runs 🤔

[mavolin]

I have updated my comment.

[mavolin]

Regardless of the differences, should I name the int8 nullable type Uint7 or Uint8, considering that the 8th bit can't really be used.

[mavolin]

As this also fits the PR, should I pointerrize/use the respective option types for all optional fields, as some haven't been changed yet?

[diamondburned]

Yeah, I think that'll do (RE use respective option types). I think Uint7 is a weird name, I'd use option.Enum or something. Int8 and Uint7 are both kind of weird names, I think.

[mavolin]

So no nullable package after all, but instead both types in option?

[diamondburned]

I think a null package is fine. I'm fine with either, really.