feat(gogenerate): add optional fields in log with omitempty annotation #619

iamrz1 · 2025-01-22T07:46:24Z

What this PR does / why we need it

gobox cannot be re-stenciled due to manual change in autogenerated file where a field was made optional. this PR adds omitempty annotation to log tags, making it possible to mark fields optional.

Jira ID

DT-4572

Notes for your reviewers

Signed-off-by: T M Rezoan Tamal <[email protected]>

malept

Perhaps there should be a test for the changes you've made in processStruct?

Also, the new log annotation should be documented in tools/logger/generating.md.

malept · 2025-01-22T16:26:03Z

tools/logger/logger.go

+			case strings.HasSuffix(field, tagOmitEmpty):
+				args["key"] = strings.TrimSuffix(field, tagOmitEmpty)
+				write(w, getSimpleOptionalFieldFormat(s.Field(kk).Type()), args)


suggestion: I think the parsing needs to happen after the if field, ok := [...] line (line 139) where you parse out the field and annotations, something like:

parts := strings.Split(",") field = parts[0] annotations := parts[1:] // I don't remember if this causes a panic

malept · 2025-01-22T16:58:18Z

tools/logger/logger.go

+	return fmt.Sprintf(`
+if s.{{.name}} != %s {
+	addField("{{.key}}", s.{{.name}})
+}`, defaultValue)


suggestion: perhaps this string literal should be moved to the same area as the other templated strings (at the top of the file)

I did that at first, but it didn't feel very intuitive because of the %s specifier. I will make this change, but I'm not really sure about it.

…ssStruct()

malept · 2025-01-23T16:02:41Z

tools/logger/generating.md

@@ -51,6 +51,41 @@ func (s *OrgInfo) MarshalLog(addField func(key string, value interface{})) {
 }
 ```

+### Annotations:


suggestion: don't put the trailing colon in the header, it doesn't make sense in the autogenerated table of contents.

Suggested change

### Annotations:

### Annotations

malept · 2025-01-23T16:28:25Z

tools/logger/generating.md

+    type OrgInfo struct
+        Org  string `log:"or.org.shortname,omitempty"`
+        Guid *Custom `log:"or.org.guid,omitempty"`


Nit: examples should follow naming conventions. In particular:

the Info in OrgInfo is superfluous

Org is duplicative of the struct name

Guid should be in all caps since it's an acronym

Although I guess you need to fix it in the rest of the docs, too.

Suggested change

type OrgInfo struct

Org string `log:"or.org.shortname,omitempty"`

Guid *Custom `log:"or.org.guid,omitempty"`

type Org struct {

Shortname string `log:"or.org.shortname,omitempty"`

GUID *Custom `log:"or.org.guid,omitempty"`

malept · 2025-01-23T16:52:50Z

tools/logger/generating.md

+    }
+    ```
+
+    Generated code:


Nit: should probably note that the generated code here is post-formatted with gofmt

malept · 2025-01-24T16:41:13Z

tools/logger/generating.md

+These tags for fields also have limited support for annotations: `ie. log:"xyz,annotation"`.
+


suggestion: this line is superfluous.

Suggested change

These tags for fields also have limited support for annotations: `ie. log:"xyz,annotation"`.

malept · 2025-01-24T16:42:30Z

tools/logger/generating.md

+
+Supported annotations:
+
+- **omitempty**: makes the field optional.


suggestion: formatting

Suggested change

- **omitempty**: makes the field optional.

- `omitempty`: makes the field optional.

tools/logger/logger.go

malept · 2025-01-24T16:53:04Z

tools/logger/generating.md

+    `omitempty` annotation is available for simple built-in types or
+    custom types with underlying type being built-in or pointers of any type.


suggestion: avoid repeating yourself, grammar, use common terminology (Go calls them "basic types")

Suggested change

`omitempty` annotation is available for simple built-in types or

custom types with underlying type being built-in or pointers of any type.

This is available for basic types, types aliased to a basic type,

and pointers of any type. The annotation parser does **not** validate

that the annotation is only applied to supported types.

malept · 2025-01-24T16:55:46Z

tools/logger/generating.md

@@ -27,7 +27,7 @@ type OrgInfo struct

 ```

-With the addition of the annotation (which matches the [standard
+With the addition of the log tags `ie. log:"xyz"` for fields (xyz matches the [standard


suggestion: wording

Suggested change

With the addition of the log tags `ie. log:"xyz"` for fields (xyz matches the [standard

With the addition of the `log` tags for fields (which matches the [standard

malept · 2025-01-24T16:59:59Z

tools/logger/logger.go

@@ -168,3 +187,31 @@ func usage() {
 	fmt.Fprintf(os.Stderr, "Flags:\n")
 	flag.PrintDefaults()
 }
+
+func getSimpleOptionalFieldFormat(p types.Type) string {


question: is this format "simple" if it also covers pointers?

Probably not anymore.

malept · 2025-01-25T01:40:31Z

tools/logger/logger_test.go

+			expected:       true,
+		},
+		{
+			name:           "uninitialized string slice",


suggestion: wrong test name?

Suggested change

name: "uninitialized string slice",

name: "uninitialized int slice",

Yeah, copy paste.

malept · 2025-01-27T16:32:31Z

@iamrz1 in the future, please avoid force pushing to PRs which have been reviewed. It makes it much more difficult to view commits which have been made since the last review. All repos in this org are set to "Squash and Merge" anyway.

iamrz1 · 2025-01-28T06:10:20Z

@malept Got it.

malept

This seems fine now, I just don't like the maintainability / readability of the ProcessStruct table test.

malept · 2025-01-28T15:50:10Z

tools/logger/logger_test.go

+				return types.NewStruct(fields, tags), "BasicStruct"
+			},
+			expected: `
+func (s *BasicStruct) MarshalLog(addField func(key string, value interface{})) {
+	if s == nil {
+		return
+	}
+
+addField("name", s.Name)
+addField("age", s.Age)
+}
+`,
+		},
+		{
+			name: "time field",
+			setup: func() (*types.Struct, string) {
+				pkg := types.NewPackage("time", "time")
+				timeType := types.NewNamed(types.NewTypeName(0, pkg, "Time", nil), &types.Struct{}, nil)
+
+				fields := []*types.Var{
+					types.NewField(0, nil, "CreatedAt", timeType, false),
+				}
+				tags := []string{`log:"created_at"`}
+				return types.NewStruct(fields, tags), "TimeStruct"
+			},
+			expected: `
+func (s *TimeStruct) MarshalLog(addField func(key string, value interface{})) {
+	if s == nil {
+		return
+	}
+
+addField("created_at", s.CreatedAt.UTC().Format(time.RFC3339Nano))
+}
+`,
+		},
+		{
+			name: "omitempty fields",
+			setup: func() (*types.Struct, string) {
+				fields := []*types.Var{
+					types.NewField(0, nil, "Name", types.Typ[types.String], false),
+					types.NewField(0, nil, "AgeP", types.NewPointer(types.Typ[types.Int]), false),
+				}
+				tags := []string{
+					`log:"name,omitempty"`,
+					`log:"ageP,omitempty"`,
+				}
+				return types.NewStruct(fields, tags), "OmitStruct"
+			},
+			expected: `
+func (s *OmitStruct) MarshalLog(addField func(key string, value interface{})) {
+	if s == nil {
+		return
+	}
+
+if s.Name != "" {
+	addField("name", s.Name)
+}
+if s.AgeP != nil {
+	addField("ageP", s.AgeP)
+}
+}
+`,
+		},
+		{
+			name: "nested marshaler",
+			setup: func() (*types.Struct, string) {
+				nestedType := types.NewPointer(types.NewNamed(
+					types.NewTypeName(0, nil, "NestedStruct", nil),
+					&types.Struct{},
+					nil,
+				))
+				fields := []*types.Var{
+					types.NewField(0, nil, "Nested", nestedType, false),
+				}
+				tags := []string{`log:"."`}
+				return types.NewStruct(fields, tags), "ParentStruct"
+			},
+			expected: `
+func (s *ParentStruct) MarshalLog(addField func(key string, value interface{})) {
+	if s == nil {
+		return
+	}
+
+if s.Nested != nil {
+	s.Nested.MarshalLog(addField)
+}
+}
+`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			s, name := tt.setup()
+			buf := &bytes.Buffer{}
+			processStruct(buf, s, name)
+			assert.Equal(t, tt.expected, buf.String())
+		})
+	}
+}


thought: I don't agree that this should be a table test - the setup function makes it pretty complex to read.

iamrz1 requested a review from a team as a code owner January 22, 2025 07:46

iamrz1 force-pushed the rezoan/DT-4572 branch from 95f8447 to a0135de Compare January 22, 2025 07:50

iamrz1 changed the title ~~feat(gogenrate): add optional fields in log with omitempty tag~~ feat(gogenerate): add optional fields in log with omitempty annotation Jan 22, 2025

feat(gogenerate): add optional fields in log with omitempty annotation

599b03a

Signed-off-by: T M Rezoan Tamal <[email protected]>

iamrz1 force-pushed the rezoan/DT-4572 branch from a0135de to 599b03a Compare January 22, 2025 08:02

malept requested changes Jan 22, 2025

View reviewed changes

iamrz1 added 3 commits January 23, 2025 16:21

feat(test): improve anoonation code and add unit tests to cover proce…

ffe256f

…ssStruct()

feat(linter): goimports and nestingReduce:

c2e3208

fix(omitempty): use array instead of strings to list annotations

2c2c511

iamrz1 requested a review from malept January 23, 2025 15:47

malept requested changes Jan 24, 2025

View reviewed changes

fix(doc): rename fields and update docs

4091e24

iamrz1 requested a review from malept January 25, 2025 01:02

malept reviewed Jan 25, 2025

View reviewed changes

fix(test): fix worng name for test

b474243

iamrz1 force-pushed the rezoan/DT-4572 branch from 152359d to b474243 Compare January 25, 2025 04:19

iamrz1 requested a review from malept January 25, 2025 04:20

malept approved these changes Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gogenerate): add optional fields in log with omitempty annotation #619

feat(gogenerate): add optional fields in log with omitempty annotation #619

iamrz1 commented Jan 22, 2025 •

edited

Loading

malept left a comment

malept Jan 22, 2025

malept Jan 22, 2025

iamrz1 Jan 23, 2025

malept Jan 23, 2025

malept Jan 23, 2025

malept Jan 23, 2025

malept Jan 24, 2025

malept Jan 24, 2025

malept Jan 24, 2025

malept Jan 24, 2025

malept Jan 24, 2025

iamrz1 Jan 25, 2025

malept Jan 25, 2025

iamrz1 Jan 25, 2025

malept commented Jan 27, 2025

iamrz1 commented Jan 28, 2025

malept left a comment

malept Jan 28, 2025

		These tags for fields also have limited support for annotations: `ie. log:"xyz,annotation"`.


		Supported annotations:

		- omitempty: makes the field optional.

	- omitempty: makes the field optional.
	- `omitempty`: makes the field optional.

		`omitempty` annotation is available for simple built-in types or
		custom types with underlying type being built-in or pointers of any type.

-    `omitempty` annotation is available for simple built-in types or
-    custom types with underlying type being built-in or pointers of any type.
+    This is available for basic types, types aliased to a basic type,
+    and pointers of any type. The annotation parser does **not** validate
+    that the annotation is only applied to supported types.

	With the addition of the log tags `ie. log:"xyz"` for fields (xyz matches the [standard
	With the addition of the `log` tags for fields (which matches the [standard

	name: "uninitialized string slice",
	name: "uninitialized int slice",

feat(gogenerate): add optional fields in log with omitempty annotation #619

Are you sure you want to change the base?

feat(gogenerate): add optional fields in log with omitempty annotation #619

Conversation

iamrz1 commented Jan 22, 2025 • edited Loading

What this PR does / why we need it

Jira ID

Notes for your reviewers

malept left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malept commented Jan 27, 2025

iamrz1 commented Jan 28, 2025

malept left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamrz1 commented Jan 22, 2025 •

edited

Loading