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

Clarify example "Export boundary functions" #1221

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JamesSlocumIH
Copy link

The following example is confusing.

  • If Config is supposed to be a parameter object, it should be named Params since the constructor is called New(...) and needs defined fx.In within.
  • If it is not meant to be an parameter object, I think passing Params into the constructor without defining that in the example is confusing.
    • Maybe we should be passing a Config instead? i.e. New(c Config)

I changed the example to use a parameter object, because I think using the parameter object makes sense here. I think if its not supposed to use a parameter object, updating the names of Params or Config to align could help make the example more clear.

I don't think the example follows good coding conventions. If `Config` is supposed to be a parameter object, it should be named `Params` since the constructor is called `New(...)` and needs defined `fx.In` within. If it is not meant to be an parameter object, I think passing `Params` into the constructor without defining that in the example is confusing.
@CLAassistant
Copy link

CLAassistant commented Jun 27, 2024

CLA assistant check
All committers have signed the CLA.

Comment on lines +136 to 139
type Params struct {
fx.In

Addr string `yaml:"addr"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config isn't intended to be a parameter object. It is being passed into New through a Params struct.
Although I agree that this example is a tad confusing: Params and parseConfig aren't visible, and we just assume that a reader knows what they are.

The text in this example is generated automatically from ex/modules/module.go; it may require some adjustment in the snippets there to update this.

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

Successfully merging this pull request may close these issues.

3 participants