AliasMangler #95

sergiosalvatore · 2024-08-22T13:53:24Z

AliasMangler

Add a new mangler that allows us to have aliases for dials tags to
facilitate migrating from one name to another seamlessly. If both the
old name and the new name are set, an error will be returned because the
expectation is that you're using one or the other exclusively.

Also, fix a bug in ez where an error thrown by a Source may cause the
process to hang.

And, lastly, specifically exclude TextUnmarshaler implementing types
from mangling.

dfinkel

a few comments

dfinkel · 2024-08-22T14:27:21Z

ez/ez_test.go

+	envErr := os.Setenv("ALTCONFIGPATH", "../testhelper/testconfig.yaml")
+	require.NoError(t, envErr)
+	defer os.Unsetenv("ALTCONFIGPATH")


you can use t.SetEnv here. (available since go 1.17)

dfinkel · 2024-08-29T16:19:07Z

README.md

+and `dialspflag` as well by appending "alias" to the flag name.  The `ez`
+package also wraps the appropriate file source so config files can also make use
+of aliases.


Suggested change

and `dialspflag` as well by appending "alias" to the flag name. The `ez`

package also wraps the appropriate file source so config files can also make use

of aliases.

and `dialspflag` as well by appending "alias" to the flag name. The `ez`

package also wraps the appropriate file source's decoder so config files can also make use

of aliases.

dfinkel · 2024-08-29T16:21:07Z

README.md

+Dials supports aliases for fields where you want to change the name and support
+an orderly transition from an old name to a new name.  Just as there is a
+`dials` struct tag there is also a `dialsalias` struct tag that you can use as
+an alternate name.  Any other casing or transformation rules on the original


I think we should mention that this functionality is provided by a mangler upfront, so anyone who doesn't use the ez package isn't confused when it doesn't work out of the box.
(I think there's a section in the README that you can link to that describes manglers -- we should add on if there isn't)

dfinkel · 2024-08-29T16:23:38Z

ez/ez.go

+	// If file-watching is not enabled, we should shutdown the monitor
+	// goroutine when exiting this function.
+	// Usually `dials.Config` is smart enough not to start a monitor when
+	// there are no `Watcher` implementations in the source-list, but the
+	// `Blank` source uses `Watcher` for its core functionality, so we need
+	// to shutdown the blank source to actually clean up resources.
+	if !params.WatchConfigFile {
+		defer blank.Done(ctx)
+	}
+


Can we split this (unrelated) bug-fix into a different commit? (it can be the same PR, but it's super-confusing to have a move that's fixing a bug in the middle of a commit introducing a new feature)

dfinkel · 2024-08-29T16:26:17Z

ez/ez.go

@@ -219,7 +219,8 @@ func ConfigFileEnvFlagDecoderFactoryParams[T any, TP ConfigWithConfigPath[T]](ct
 		return nil, fmt.Errorf("decoderFactory provided a nil decoder for path: %s", cfgPath)
 	}

-	manglers := make([]transform.Mangler, 0, 2)
+	manglers := make([]transform.Mangler, 0, 3)
+	manglers = append(manglers, &transform.AliasMangler{})


nit: drop the &.
AliasMangler is a zero-size struct, and doesn't need an address.
The go runtime/compiler will dedup zero-size-typed addresses in a lot of cases, including interface assignments. Storing the pointer type does us zero good here.

dfinkel · 2024-08-29T16:34:39Z

transform/alias_mangler.go

+// conveniently.
+type AliasMangler struct{}
+
+// Mangle implements the Mangler interface.  If an alias tag is defined, the


nit: s/an/any/. (an is a little ambiguous in the case where multiple tags are set)

dfinkel · 2024-08-29T16:46:52Z

transform/alias_mangler.go

+				Name: desc.Name + " (alias of `" + originalVals[tag] + "`)",
+			}
+			if setErr := tags.Set(newDesc); setErr != nil {
+				return nil, fmt.Errorf("error setting amended dialsdesc for tag %q: %w", tag, setErr)


I hate it when people try to make APIs too "safe".
The only existing case where tags.Set returns an error is when the Key field is empty .

I wonder how much code we'd be able to delete if we used a library that paniced when you do something that stupid instead of forcing everyone to handle an error that cannot happen in any case where you have the least bit of control.

dfinkel · 2024-08-29T16:50:23Z

transform/alias_mangler.go

+		// update dialsdesc if there is one
+		if desc, getErr := tags.Get("dialsdesc"); getErr == nil {
+			newDesc := &structtag.Tag{
+				Key:  "dialsdesc",
+				Name: desc.Name + " (alias of `" + originalVals[tag] + "`)",
+			}
+			if setErr := tags.Set(newDesc); setErr != nil {


Hmm, this dialsdesc handling should probably be outside the tag loop. I think as-is, it'll clobber the tag multiple times if several of the alias tags are set at once. (which I think we want to allow)

As discussed on Slack, I need to leave some accumulation in the loop and then concatenate all the options for the dialsdesc tag afterwards.

dfinkel · 2024-08-29T16:51:43Z

transform/alias_mangler.go

+		}
+	}
+
+	// set the new flags on the alias field


nit: s/flags/tags/

dfinkel · 2024-08-29T16:53:45Z

transform/transformer.go

+		// also don't recurse into TextUnarshaler types
+		if ft.Implements(textMReflectType) || reflect.PointerTo(ft).Implements(textMReflectType) {
+			continue
+		}


We should split this into a separate commit.

This fixes a bug in ez where an error thrown by a Source may cause the process to hang.

We shouldn't ever mangle types that implement TextUnmarshaler.

dfinkel

one nit

dfinkel · 2024-08-29T21:24:01Z

README.md

+
+Aliases are implemented using the
+[Mangler](https://github.com/vimeo/dials/blob/402b4821e3f191d96580b9933583f1651325381d/transform/transformer.go#L17-L32)
+interface.  The `env`, `flag`, and `pflag` support aliasing without any


nit:

Suggested change

interface. The `env`, `flag`, and `pflag` support aliasing without any

interface. The `env`, `flag`, and `pflag` sources support aliasing without any

Good catch. Fixed.

Add a new mangler that allows us to have aliases for dials tags to facilitate migrating from one name to another seamlessly. If both the old name and the new name are set, an error will be returned because the expectation is that you're using one or the other exclusively.

sergiosalvatore force-pushed the alias-tag branch from 047e3d9 to 0ff99d6 Compare August 23, 2024 15:43

sergiosalvatore changed the title ~~wip~~ AliasMangler Aug 23, 2024

sergiosalvatore force-pushed the alias-tag branch 4 times, most recently from 5d5cfaa to 40b8af1 Compare August 23, 2024 20:09

sergiosalvatore marked this pull request as ready for review August 23, 2024 20:14

dfinkel reviewed Aug 29, 2024

View reviewed changes

sergiosalvatore force-pushed the alias-tag branch from 40b8af1 to f7a1949 Compare August 29, 2024 21:06

sergiosalvatore added 2 commits August 29, 2024 17:07

Fix Source Error Hang

4f66430

This fixes a bug in ez where an error thrown by a Source may cause the process to hang.

TextUnmarshaler is Excluded from Mangling

dbcdd5f

We shouldn't ever mangle types that implement TextUnmarshaler.

sergiosalvatore force-pushed the alias-tag branch 3 times, most recently from 55f2b40 to 8e387cb Compare August 29, 2024 21:12

dfinkel approved these changes Aug 29, 2024

View reviewed changes

AliasMangler

57e6f14

Add a new mangler that allows us to have aliases for dials tags to facilitate migrating from one name to another seamlessly. If both the old name and the new name are set, an error will be returned because the expectation is that you're using one or the other exclusively.

sergiosalvatore force-pushed the alias-tag branch from 8e387cb to 57e6f14 Compare August 30, 2024 13:07

sergiosalvatore merged commit 2e8ffb1 into master Aug 30, 2024
8 checks passed

sergiosalvatore deleted the alias-tag branch August 30, 2024 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AliasMangler #95

AliasMangler #95

sergiosalvatore commented Aug 22, 2024 •

edited

Loading

dfinkel left a comment

dfinkel Aug 22, 2024

dfinkel Aug 29, 2024

dfinkel Aug 29, 2024

dfinkel Aug 29, 2024

dfinkel Aug 29, 2024

dfinkel Aug 29, 2024

dfinkel Aug 29, 2024

dfinkel Aug 29, 2024

sergiosalvatore Aug 29, 2024

dfinkel Aug 29, 2024

dfinkel Aug 29, 2024

dfinkel left a comment

dfinkel Aug 29, 2024

sergiosalvatore Aug 30, 2024

	interface. The `env`, `flag`, and `pflag` support aliasing without any
	interface. The `env`, `flag`, and `pflag` sources support aliasing without any

AliasMangler #95

AliasMangler #95

Conversation

sergiosalvatore commented Aug 22, 2024 • edited Loading

dfinkel 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

dfinkel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiosalvatore commented Aug 22, 2024 •

edited

Loading