dig icon indicating copy to clipboard operation
dig copied to clipboard

Feature request: hints for pointer vs struct in value groups

Open kriskowal opened this issue 6 years ago • 4 comments

We do a great job hinting when you provide a struct instead of a pointer to a struct, or the other way around. However, this magic does not appear to extend all the way to value groups.

I was stumped by a test:

	var p struct {
		fx.In

		ProcedureLists [][]*transport.Procedure `group:"yarpcfx"`
	}

This did not work because the value group contains slices of transport.Procedure structs, not pointers thereto.

Would be great if the Fx output hinted that it would populate the ProcedureLists were it without the pointer indirection.

kriskowal avatar Mar 08 '18 23:03 kriskowal

Ah, good catch.

HelloGrayson avatar Mar 08 '18 23:03 HelloGrayson

If anyone wants to get their hands dirty with dig, this should be fun to implement. It's all contained in newErrMissingType.

abhinav avatar Mar 08 '18 23:03 abhinav

Hi. Been trying to implement this is in newErrMissingType as suggested by @abhinav but the problem, as I've described in #389, is that this method is never called when you don't provide any constructors for a value group. Should we add a check for no constructors for value groups? Otherwise we'll need to go further up the call stack to check for this.

paullen avatar Mar 18 '24 05:03 paullen

I do think that this would be a useful check to add, but my concerns are:

  • I don't think it is necessarily invalid to have an empty value group, though you could make the argument that an empty value group is very likely to be unintentional when the opposite kind (pointer vs. value) was detected as being provided.
  • It would break current users who DO have empty value groups

I think if we do add this, we should probably gate it behind an option, dig.FailEmptyValueGroups, or something like that, but curious to hear others' thoughts.

JacobOaks avatar Mar 19 '24 20:03 JacobOaks