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

feat: expose Dialer and add DialerOptions #7

Merged
merged 6 commits into from
Apr 2, 2021
Merged

Conversation

kurtisvg
Copy link
Contributor

Exports a Dialer object that can be used to configure the Dialer behavior.

@kurtisvg kurtisvg requested a review from enocom March 30, 2021 18:12
@kurtisvg kurtisvg changed the title export the Dialer object and provide options for configuration feat: expose the Dialer and DialerOption objects Mar 30, 2021
@kurtisvg kurtisvg changed the title feat: expose the Dialer and DialerOption objects feat: expose Dialer and DialerOption objects Mar 30, 2021
@kurtisvg kurtisvg changed the title feat: expose Dialer and DialerOption objects feat: expose Dialer and add DialerOption Mar 31, 2021
@kurtisvg kurtisvg changed the title feat: expose Dialer and add DialerOption feat: expose Dialer and add DialerOptions Mar 31, 2021
options.go Outdated
}
}

// WithCredentialsFile returns a DialerOption that specifies a service account or refresh token JSON credentials to be used as the basis for authentication.
Copy link
Member

Choose a reason for hiding this comment

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

This docstring is identical to the one above. Maybe we just crib from the options package for these functions since they're basically call-throughs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean by crib - can you expand?

If you mean just housing the options in the api/options package and asking for a ClientOptions type in the Dialer instead, I didn't do that because there will be non-client specific options in the future.

If you mean adding a WithClientOption that allows the user to specify a client option, it felt both clunkier to double wrap options but I'm not sure we want to expose all ClientOptions to the user. This way lets us limit which Options can be used, and lets us potentially add more complexity to under the hood to how these options are applied (if we need too).

Copy link
Member

Choose a reason for hiding this comment

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

I'm just suggesting we tighten up the docstring here to call out the difference with the function below.

@kurtisvg kurtisvg requested a review from enocom March 31, 2021 17:22
// defaultDialer provides the singleton dialer as a default for dial functions.
func defaultDialer() (*dialManager, error) {
// getDefaultDialer provides the singleton dialer as a default for dial functions.
func getDefaultDialer() (*Dialer, error) {
Copy link
Member

Choose a reason for hiding this comment

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

@kurtisvg kurtisvg merged commit 1235a9f into main Apr 2, 2021
@kurtisvg kurtisvg deleted the dialer_export branch April 2, 2021 22:28
This was referenced Apr 5, 2022
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 this pull request may close these issues.

2 participants