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

add isEmptyStruct func #362

Closed
wants to merge 1 commit into from
Closed

Conversation

kkHAIKE
Copy link
Contributor

@kkHAIKE kkHAIKE commented Aug 5, 2022

related to
#360

@arp242
Copy link
Collaborator

arp242 commented Aug 5, 2022

The isEmptyStruct() implementation doesn't just check if it's an empty struct, but also if it's an empty array, slice, or map, and the Array implementation is different from isEmpty:

type Struct struct {
	Arr  [3]string `toml:",omitempty"`  // Never empty as len always longer

	// But this behaves different, as it will check the zero value of Arr.
	Nest struct {
		Arr [3]string
	} `toml:",omitempty"`
}

In principle the second implementation is better, but right now it's documented as this for Encoder:

The toml struct tag can be used to provide the key name; if omitted the
struct field name will be used. If the "omitempty" option is present the
following value will be skipped:

  - arrays, slices, maps, and string with len of 0
  - struct with all zero values
  - bool false

I think we can change it without breaking compatibility; now omitempty on an array is basically useless just as it was for structs before the last release. But it should be identical throughout (and the documentation updated, and test added).

I think re-using isEmpty() for slices, maps, and arrays rather doing it in isEmptyStruct() is better, as it avoids implementations differing.


Also, I think the tests should be expanded, for example:

type Nest struct {
	Field struct {
		uncomp Uncomparable
	}
}

type Embed struct {
	Uncomparable
}

type EmbedNest struct {
	Field struct {
		Uncomparable
	}
}

type UncompSlice struct {
	[]Uncomparable
}

type UncompSliceNest struct {
	Field struct {
		uncomp []Uncomparable
	}
}

Maybe some more? That's all the cases I could think of right now.

@arp242
Copy link
Collaborator

arp242 commented Aug 5, 2022

I think changing the array behaviour would be done best in a different PR btw, and encoding/json has the same check:

case reflect.Array, reflect.Map, reflect.Slice, reflect.String:
	return v.Len() == 0

So I'm not sure yet if we should change it.

@arp242
Copy link
Collaborator

arp242 commented Aug 5, 2022

Discussion about that on encoding/json: golang/go#29310

@kkHAIKE
Copy link
Contributor Author

kkHAIKE commented Aug 6, 2022

'omitempty' from encoding/json not work on struct type, so isEmpty only check array.len==0 is well.

Actually, original toml's behaviour 'reflect.Zero(rv.Type()).Interface() == rv.Interface()' is same as 'rv.IsZero()', because 'reflect.Zero' is make a zero value struct

I think 'isEmptyStruct' behaviour for Array type struct field is right, because 'reflect.Zero' can make a all zero value Array, not len==0 Array.

  • struct with all zero values
    change to 'rv.IsZero()' simply ? it work with Uncomparable

@arp242
Copy link
Collaborator

arp242 commented Oct 22, 2022

Fixed via #366

@arp242 arp242 closed this Oct 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants