feat: Handle omitzero in structfield linter

[Not-Dhananjay-Mishra]

Added handling for the ,omitzero tag option in processTag (structfield.go) by stripping it from the tag value before further validation, consistent with existing ,omitempty behavior.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.54%. Comparing base (8ca48dd) to head (2624e99).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files

@@ Coverage Diff @@## master #3881 +/- ##
==========================================
+ Coverage 92.48% 92.54% +0.05% 
==========================================
Files 200 200 Lines 14564 14679 +115 ==========================================
+ Hits 13469 13584 +115 
Misses 895 895 Partials 200 200 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gmlewis]

Thank you, @Not-Dhananjay-Mishra!
Please add test cases in all the test data that includes new ",omitzero" and ",omitempty,omitzero". I see you added one, but we need examples for all 4 possibilities. (warning, non-warning, single flag, double flags)

[Not-Dhananjay-Mishra]

@gmlewis I added some test cases

No warning -

• SliceOfZerosStructs []*Struct json:"slice_of_zeros_structs,omitzero"
• SliceOfBothOmitTags []*Struct json:"slice_of_both_omit_tags,omitempty,omitzero"

Has warning -

• SliceOfZeroInts []*int json:"slice_of_zero_ints,omitzero"
• SliceOfBothOmitTags []*int json:"slice_of_both_omit_tags,omitempty,omitzero"
• PerPageZeros int url:"per_page_zeros,omitzero"
• PerPageBoth int url:"per_page_both,omitempty,omitzero"

[Not-Dhananjay-Mishra]

Just to confirm, are we going to use omitzero with the following conditions?

• Slice of pointer structs
• Slice of non-pointer data types (int, string)
• Pointer data types

[gmlewis]

Just to confirm, are we going to use omitzero with the following conditions?

• Slice of pointer structs
• Slice of non-pointer data types (int, string)
• Pointer data types

HAH! We were typing at the same time!

Excellent questions!!! I don't know the answers. I'd like to hear some more opinions.

[Not-Dhananjay-Mishra]

Now omitzero is :

Allowed with -

• []builtin & []*builtin
• map & *map
• Struct & *Struct
• []Struct & []*Struct

Not Allowed with -

• with URL Tag
• builtin & *builtin
• *[]builtin/Struct & *[]*builtin/Struct

[Not-Dhananjay-Mishra]

@alexandear could you please confirm whether the current implementation aligns with your suggestion, or if any changes are needed?

[gmlewis]

I'm having troubles with:

• *map[...]...
• Struct
• []Struct

I still think these should not be allowed, but instead use:

• map[...]...
• *Struct
• []*Struct

But maybe I'm just not getting the whole thing?

cc: @stevehipwell - @alexandear - @zyfy29 - please help me make sense of how we should handle omitzero in this repo, as I'm really struggling with this one.

[Not-Dhananjay-Mishra]

Should I go ahead with this :

Allowing only - map[...]... , *Struct , []*Struct and []buildin as suggested.

I tried a few small experiments with different field shapes. From what I observed, even when the data is “empty”, these values are still considered non-zero and therefore are not omitted during marshaling.

I tested with the following example.

typeDummyDataForOmitZerostruct {
OmitZeroWithoutPointerSlice []int`json:"omit_zero_without_pointer_slice,omitzero"`OmitZeroWithPointerSlice []*int`json:"omit_zero_with_pointer_slice,omitzero"`OmitZeroWithoutPointerStructStruct`json:"omit_zero_without_pointer_struct,omitzero"`OmitZeroWithPointerStruct*Struct`json:"omit_zero_with_pointer_struct,omitzero"`OmitZeroWithoutPointerSliceofStruct []Struct`json:"omit_zero_without_pointer_sliceofstruct,omitzero"`OmitZeroWithPointerSliceofStruct []*Struct`json:"omit_zero_with_pointer_sliceofstruct,omitzero"`OmitZeroWithoutPointerMapmap[string]string`json:"omit_zero_without_pointer_map,omitzero"`OmitZeroWithPointerMap*map[string]string`json:"omit_zero_with_pointer_map,omitzero"`
}

Empty input:

data:=DummyDataForOmitZero{
OmitZeroWithoutPointerSlice: []int{},
OmitZeroWithPointerSlice: []*int{},
OmitZeroWithoutPointerSliceofStruct: []Struct{},
OmitZeroWithPointerSliceofStruct: []*Struct{},
OmitZeroWithoutPointerStruct: Struct{},
OmitZeroWithPointerStruct: &Struct{},
OmitZeroWithoutPointerMap: map[string]string{},
OmitZeroWithPointerMap: &map[string]string{},
}

Marshaled output:

{
"omit_zero_without_pointer_slice": [],
"omit_zero_with_pointer_slice": [],
"omit_zero_with_pointer_struct": {},
"omit_zero_without_pointer_sliceofstruct": [],
"omit_zero_with_pointer_sliceofstruct": [],
"omit_zero_without_pointer_map": {},
"omit_zero_with_pointer_map": {}
}

or should non-pointer values be allowed as well?
any opinion?

[gmlewis]

Awesome... thank you for the excellent experiment... that is useful to see the results.

Here's my opinion, and I am totally happy to be corrected or discuss:

I have some ideas of what should never be used in this repo (regardless the JSON tags) unless there is a REALLY GOOD reason (which, granted, does indeed sometimes happen):

• pointers to slices or maps (which are already reference types)
• slices of pointers to primitives
• slices of structs without the pointers (for iteration)

As for the use of omitzero, I don't have a good enough "feel" for them yet to really understand their use.

Looking carefully at the above experiment, only one case was removed by omitzero:

• OmitZeroWithoutPointerStruct Struct json:"omit_zero_without_pointer_struct,omitzero"

But I'm struggling to figure out if that is a really useful case in this repo. It seems like a very targeted and specific use case... one that we might indeed find useful, but honestly I don't know where.

So in my opinion, if we still don't allow my list above, then I think we may be able to proceed.

[Not-Dhananjay-Mishra]

make sense.
I’ll proceed with -

Allowing only

• map[...]...
• *Struct
• []*Struct
• []builtin (e.g. []int, []string .... )

We can adjust the linter later if necessary.

[gmlewis]

Thank you, @Not-Dhananjay-Mishra for this PR and for your patience while I have been trying to figure out what makes sense to me.
I'm liking this a lot.
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.

cc: @stevehipwell - @alexandear - @zyfy29

[gmlewis]

Thank you, @alexandear!
Merging.