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

Standardize params vs configuration struct naming #122

Closed
bufdev opened this issue Mar 4, 2022 · 1 comment · Fixed by #133
Closed

Standardize params vs configuration struct naming #122

bufdev opened this issue Mar 4, 2022 · 1 comment · Fixed by #133

Comments

@bufdev
Copy link
Member

bufdev commented Mar 4, 2022

I think the purpose of all of these is (relatively) similar:

handler.go:23:type handlerConfiguration struct {
client.go:22:type clientConfiguration struct {
internal/assert/assert.go:187:type optionFunc func(*params)
internal/assert/assert.go:191:type params struct {
protocol.go:47:type protocolHandlerParams struct {
protocol.go:90:type protocolClientParams struct {

I'd argue for params.

A note, I think protocolHandlerParams/protocolClientParams are relics of when grpc was a separate package, is this right (was it)? I think they likely still make sense given the number of fields though.

Also a note, I don't think assert.params actually buys us what we want - might want to limit it to the options and just pass the parameters in directly (in every function, we call newParams anyways). Super minor nit though.

@akshayjshah
Copy link
Member

A note, I think protocolHandlerParams/protocolClientParams are relics of when grpc was a separate package, is this right (was it)? I think they likely still make sense given the number of fields though.

No, it's just a lot of stuff to pass without names. Functional options for unexported APIs also seemed over-engineered.

Also a note, I don't think assert.params actually buys us what we want - might want to limit it to the options and just pass the parameters in directly (in every function, we call newParams anyways). Super minor nit though.

Good point, will open a PR to fix. I think we can probably clean up assert quite a bit - it originally supported a much larger set of features, most of which we're not using anymore.

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

Successfully merging a pull request may close this issue.

2 participants