Add struct tag support for field matching #164

kb-sp · 2024-11-09T11:44:14Z

This PR adds support for using struct tags to match variable names.

Feedback welcome!

Following up on #132 this adds a new option that takes the name of the tag to use for matching. For example:

// goverter:converter
// goverter:matchTag json

Tag matching is not performed if matchTag is not set, and matching falls back to standard code paths.

Docs have been updated, and functionality seems good on my code base.

…ct tag names

…on-pb matching

codecov · 2024-11-09T13:02:39Z

Codecov Report

Attention: Patch coverage is 71.69811% with 15 lines in your changes missing coverage. Please review.

Project coverage is 95.13%. Comparing base (076c25f) to head (ecdb0b6).

Files with missing lines	Patch %	Lines
xtype/type.go	65.11%	14 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
- Coverage   96.53%   95.13%   -1.40%     
==========================================
  Files          46       44       -2     
  Lines        2250     2715     +465     
==========================================
+ Hits         2172     2583     +411     
- Misses         53      109      +56     
+ Partials       25       23       -2

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

jmattheis

Thanks for the contribution, remarks in subcomments.

jmattheis · 2024-11-09T13:27:11Z

builder/struct.go

@@ -39,6 +39,7 @@ func (*Struct) Build(gen Generator, ctx *MethodContext, sourceID *xtype.JenID, s
 	usedSourceID := false


This change should have 100% patch coverage.

jmattheis · 2024-11-09T13:29:02Z

builder/struct.go

@@ -39,6 +39,7 @@ func (*Struct) Build(gen Generator, ctx *MethodContext, sourceID *xtype.JenID, s
 	usedSourceID := false
 	for i := 0; i < target.StructType.NumFields(); i++ {


It should be allowed to define matchTag multiple times. E.g. this should work:

// goverter:converter type Converter interface { // goverter:matchTag json // goverter:matchTag yaml Convert(source Input) Output } type Input struct { A1 int `json:"a"` A2 int `json:"b"` } type Output struct { B1 int `yaml:"a"` B2 int `yaml:"b"` }

Or perhaps goverter:matchTag json yaml applied in order of precedence?

I did think about having the matchTags be []string vs string, but there's a bit of extra complexity here. This could become a bit of a NxM comparison in cases like:

type Input struct { A1 int `json:"a" yaml:"p"` A2 int `json:"b"` } type Output struct { B1 int `json:"p" yaml:"a"` B2 int `yaml:"b"` }

Does json yaml or json\nyaml\n imply precedence, in which case A1 would fail to match B1, or is the comparison exhaustive? (And would fail in the note you made about ambiguity causing an error.)

The multi-tag use case feels a bit more rare, so I was hesitant to add the complexity. But I'm happy to add it.

I agree it's likely rare, but I don't think it adds that much complexity after #164 (comment) is supported.

There shouldn't be precedence, it should error like described here #164 (comment)

Basically all/most ambiguous cases are an error in goverter. E.g.

duplicated enum values https://github.com/jmattheis/goverter/blob/main/scenario/enum_duplicated_different_values.yml

multiple field name matches

https://github.com/jmattheis/goverter/blob/main/scenario/match_ignore_case_unresolved_ambiguity.yml

https://github.com/jmattheis/goverter/blob/main/scenario/auto_map_ambigious.yml

jmattheis · 2024-11-09T13:32:02Z

builder/struct.go

@@ -39,6 +39,7 @@ func (*Struct) Build(gen Generator, ctx *MethodContext, sourceID *xtype.JenID, s
 	usedSourceID := false
 	for i := 0; i < target.StructType.NumFields(); i++ {
 		targetField := target.StructType.Field(i)


matchTag shouldn't take precedence over name matching. If there is a clash, then goverter should error. E.g. this

// goverter:converter type Converter interface { // goverter:matchTag json Convertre(source Input) Output } type Input struct { A int `json:"B"` B int `json:"A"` } type Output struct { A int `json:"B"` }

The error should be similar to this: https://github.com/jmattheis/goverter/blob/main/scenario/match_ignore_case_unresolved_ambiguity.yml

This adds complexity, so I hesitated. Again, happy to add it.

jmattheis · 2024-11-09T13:33:22Z

builder/struct.go

@@ -39,6 +39,7 @@ func (*Struct) Build(gen Generator, ctx *MethodContext, sourceID *xtype.JenID, s
 	usedSourceID := false
 	for i := 0; i < target.StructType.NumFields(); i++ {
 		targetField := target.StructType.Field(i)
+		targetTag := target.StructType.Tag(i)
 		delete(definedFields, targetField.Name())


matchTag should support matchIgnoreCase.

package example // goverter:converter type Converter interface { // goverter:matchTag json // goverter:matchIgnoreCase Convertre(source Input) Output } type Input struct { A1 int `json:"a"` } type Output struct { B1 int `json:"A"` }

IME mixed json casing is rare, with matchIgnoreCase being useful for matching "fooBar" to "FooBar" or "fooId" to "fooID". Because of the general rigor I've seen in tag naming I hesitated because of complexity, but yet again, happy to oblige. ;)

I don't think there is a clear rule how to name these, and having different casing there doesn't seem to be that rare:

userId 1134 matches https://grep.app/search?q=json%3A%22userId&regexp=true&case=true

userID 349 matches https://grep.app/search?q=json%3A%22userID&regexp=true&case=true

userid 305 matches https://grep.app/search?q=json%3A%22userid&regexp=true&case=true

UserId 75 matches https://grep.app/search?q=json%3A%22UserId&regexp=true&case=true

jmattheis · 2024-11-09T13:34:18Z

docs/reference/matchTag.md

+`matchTag TAGNAME` can be defined as [CLI argument](./define-settings.md#cli) or
+[conversion comment](./define-settings.md#conversion).


This is incorrect, it's a method comment that is inheritable. Use the text from https://goverter.jmattheis.de/reference/matchIgnoreCase

Thanks, this was something I was definitely not sure about.

jmattheis · 2024-11-09T13:36:09Z

xtype/type.go

+
+	// Protobuf tags are exciting, in that they /optionally/ have a json= section which takes
+	// precedence over the name= section.
+	if matchTag == "protobuf" {


I think this is not needed for the first version. The protobuf stuff should be already covered via json. So we don't need this custom parsing for protobuf.

The problem here is that the json: tag that is generated from tools is snake-cased, whereas it's possible to configure UseProtoNames to emit the camel-cased option in the protobuf: tag itself. Without this, I wouldn't be able to utilize tags. This is why I have the json: fallback to cover both use cases.

Hmm understood, I dislike specific implementations that aren't general in goverter and also the internal precedence from json/name protobuf key.

What do you think about a more generic implementation where the user can configure the delimiters?

type TagMatch struct { Name string Key string FieldDelim string ValueDelim string } fieldDelim=, is the default matchTag json is same as matchTag json delimiter=, matchTag protobuf valueDelim== key=name matchTag protobuf valueDelim== key=json matchTag gorm fieldDelm=; valueDelim=: key=column https://gorm.io/docs/models.html

jmattheis · 2024-11-09T13:37:19Z

scenario/matchTag.yml

@@ -0,0 +1,32 @@
+input:


This file should match the casing of the other files in this directory (snake_case.yml)

jmattheis · 2024-11-09T13:37:59Z

example/matchTag/input.go

@@ -0,0 +1,14 @@
+package example


The directory of this file should be the same format as the other directories. match-tag

jmattheis · 2024-11-09T13:38:27Z

docs/reference/matchTag.md

+`matchTag TAGNAME` can be defined as [CLI argument](./define-settings.md#cli) or
+[conversion comment](./define-settings.md#conversion).
+
+`matchTag` instructs goverter to use the given TAGNAME to exactly match fields between source and


Add how the tag is parsed. (First value of , delimited)

jmattheis · 2024-11-09T13:39:37Z

go.mod

-	golang.org/x/tools v0.17.0
+	github.com/dave/jennifer v1.7.1
+	github.com/stretchr/testify v1.9.0
+	golang.org/x/tools v0.24.0


go1.18 build is broken because of the dep update.

Sorry, I thought I brought it up just before 1.18 incompatibility and those checks weren't running for me to notice. Will fix.

kb-sp added 2 commits November 9, 2024 03:35

Initial version: add matchTag option to match struct fields with stru…

f682462

…ct tag names

update docs

5705dad

kb-sp mentioned this pull request Nov 9, 2024

Add json: (or custom) struct tag support for field matching #132

Open

kb-sp added 3 commits November 9, 2024 03:45

simplify

8f52444

add test

004ead3

have matchTag protobuf fall back to the full-fat json target for pb/n…

ecdb0b6

…on-pb matching

jmattheis requested changes Nov 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add struct tag support for field matching #164

Add struct tag support for field matching #164

kb-sp commented Nov 9, 2024 •

edited

Loading

codecov bot commented Nov 9, 2024

jmattheis left a comment

jmattheis Nov 9, 2024

jmattheis Nov 9, 2024

kb-sp Nov 9, 2024

jmattheis Nov 10, 2024

jmattheis Nov 9, 2024

kb-sp Nov 9, 2024

jmattheis Nov 9, 2024

kb-sp Nov 9, 2024

jmattheis Nov 10, 2024

jmattheis Nov 9, 2024

kb-sp Nov 9, 2024

jmattheis Nov 9, 2024

kb-sp Nov 9, 2024

jmattheis Nov 10, 2024

jmattheis Nov 9, 2024

jmattheis Nov 9, 2024

jmattheis Nov 9, 2024

jmattheis Nov 9, 2024

kb-sp Nov 9, 2024

		@@ -39,6 +39,7 @@ func (Struct) Build(gen Generator, ctx MethodContext, sourceID *xtype.JenID, s
		usedSourceID := false

		@@ -39,6 +39,7 @@ func (Struct) Build(gen Generator, ctx MethodContext, sourceID *xtype.JenID, s
		usedSourceID := false
		for i := 0; i < target.StructType.NumFields(); i++ {

		`matchTag TAGNAME` can be defined as [CLI argument](./define-settings.md#cli) or
		[conversion comment](./define-settings.md#conversion).

Add struct tag support for field matching #164

Are you sure you want to change the base?

Add struct tag support for field matching #164

Conversation

kb-sp commented Nov 9, 2024 • edited Loading

codecov bot commented Nov 9, 2024

Codecov Report

jmattheis 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

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

kb-sp commented Nov 9, 2024 •

edited

Loading