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.

Sign up for GitHub

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

Jump to bottom

CLOUDP-237245: adding helper for using Http transport interface #35

Merged
merged 2 commits into from Mar 26, 2024
Merged

CLOUDP-237245: adding helper for using Http transport interface #35

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
826af51
CLOUDP-237245: adding helper for using Http transport interface
wtrocki Mar 26, 2024
b4c6b7d
fix: comment
wtrocki Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions digest.go
View file
Open in desktop
Expand Up @@ -124,6 +124,16 @@ func NewTransportWithHTTPTransport(username, password string, transport *http.Tr
return t
}

// NewTransportWithHTTPRoundTripper creates a new digest transport using the supplied http.RoundTripper interface.
func NewTransportWithHTTPRoundTripper(username, password string, transport http.RoundTripper) *Transport {
Copy link

Choose a reason for hiding this comment

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

Is the prefix New a common convention?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes there are no constructors in go so this is the convention for constructor like behavior, also there's no method overloading so the With is somewhat common as well

Copy link
Collaborator

@gssbzn gssbzn Mar 26, 2024 •
edited

Choose a reason for hiding this comment

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

And I didn't mention this on the code review since NewTransportWithHTTPTransport already exists but both NewTransportWithHTTPTransport and the new method are not necessary can already create a new transport doing

t := &digest.Transport{
		Username:  username,
		Password:  password,
		Transport: transport,
	}

or even

t := &digest.Transport{
		username,
		password,
		transport,
	}

New methods are only needed if there are private fields or added logic but Transport has none

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My personal opinion is that New methods produce slightly better end user API as new fields in the structure would not cause compilation issues.

Example:
mongodb/atlas-sdk-go@0f2d20a

t := &Transport{
Username: username,
Password: password,
Transport: transport,
}
return t
}

type challenge struct {
Realm string
Domain string
Expand Down