Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: hints for pointer vs struct in value groups #184

Open
kriskowal opened this issue Mar 8, 2018 · 4 comments
Open

Feature request: hints for pointer vs struct in value groups #184

kriskowal opened this issue Mar 8, 2018 · 4 comments

Comments

@kriskowal
Copy link

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.

@HelloGrayson
Copy link
Contributor

Ah, good catch.

@abhinav
Copy link
Collaborator

abhinav commented Mar 8, 2018

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

@paullen
Copy link
Contributor

paullen commented Mar 18, 2024

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.

@JacobOaks
Copy link
Contributor

JacobOaks commented Mar 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants