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

Split req and client #230

Merged
merged 6 commits into from
Feb 4, 2020
Merged

Split req and client #230

merged 6 commits into from
Feb 4, 2020

Conversation

azr
Copy link
Contributor

@azr azr commented Jan 28, 2020

This PR replaces #176.

This branch split request arguments from the client into a new request struct.

This is the groundwork to allow the go-getter to use file inplace or tell what operation happened.

Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

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

Overall very good! This seems like a very nice improvement, as we can configure and share the same client instance for multiple requests.

I have a question about ClientMode and few other suggestions that you can take with a grain of salt.

client_option.go Outdated Show resolved Hide resolved
client_option.go Outdated Show resolved Hide resolved
checksum.go Outdated Show resolved Hide resolved
checksum.go Show resolved Hide resolved
request.go Show resolved Hide resolved
get.go Show resolved Hide resolved
@azr azr force-pushed the split_req_and_operation branch from 49dd117 to a8c9a40 Compare February 4, 2020 16:16
@azr
Copy link
Contributor Author

azr commented Feb 4, 2020

Thanks for reviewing @notnoop 🙏 !! Pong 🏓 !

@azr azr changed the title Split req and operation Split req and client Feb 4, 2020
Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks!

checksum.go Outdated Show resolved Hide resolved
checksumFileURL, err := urlhelper.Parse(checksumFile)
// ChecksumFromFile will only return checksums for files that match
// checksummedURL, which is the object being checksummed.
func (c *Client) ChecksumFromFile(ctx context.Context, checksumURL, checksummedURL string) (*FileChecksum, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, one more nitpick - now that we pass u.Path, checksummedURL should be checksummedPath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I didn't see this one sorry, you are correct. I will make a new PR changing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I pushed it directly to v2 🙂

@azr
Copy link
Contributor Author

azr commented Feb 4, 2020

Many thanks for the review @notnoop. Given the other positive review from #232; this can finally be merged !!!!

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