Conversation
ttaylorr
left a comment
There was a problem hiding this comment.
Looking good, just a few comments.
|
|
||
| const slash = "/" | ||
|
|
||
| func joinURL(prefix, suffix string) string { |
There was a problem hiding this comment.
Could we use path.Join instead? That should work as a drop-in replacement here 👍
There was a problem hiding this comment.
It does extra stuff we don't need (calling path.Clean()) in our limited uses of (*Client) NewRequest() that we don't need. suffix is always a simple string like "objects/batch" or "locks". Though I suppose anyone can import the package and call it with a weird suffix. Though I'm not sure why anyone would do that.
There was a problem hiding this comment.
Doesn't work without parsing the URL:
Error: Not equal: "https://example.com/a" (expected)
!= "https:///example.com/a" (actual)
| } | ||
|
|
||
| func decodeResponse(res *http.Response, obj interface{}) error { | ||
| func IsDecodeTypeError(err error) bool { |
There was a problem hiding this comment.
When adding error types to existing packages, I've usually stuck to putting them in the errors package itself, but I think defining them here for a new package is a great call 👍
There was a problem hiding this comment.
I don't think the errors package should know about all lfs errors.
| {Property: "a", Value: "A"}, | ||
| }, | ||
| Cursor: "cursor", | ||
| Limit: 5, |
There was a problem hiding this comment.
What do you think about setting a limit of 1 so we can test paginating here as well?
There was a problem hiding this comment.
Pagination isn't handled here, it's in locking.Client.
| return | ||
| } | ||
|
|
||
| if strings.Contains(r.URL.Path, "/info/lfs/locks") { |
There was a problem hiding this comment.
Does this handle unlocking? We should probably remove the two old handlers on L71-72 to make sure that we're hitting the right URL.
There was a problem hiding this comment.
I used these to make sure all requests were routed correctly: https://github.com/git-lfs/git-lfs/pull/1818/files#diff-a3b6c1ff24bf259401c990b87d408bb6R160.
There was a problem hiding this comment.
I'm not going to worry about this since master diverged when you fixed the locking relative path issue.
| return &Client{ | ||
| Remote: remote, | ||
| client: &lockClient{Client: lfsClient}, | ||
| cache: &nilLockCacher{}, |
There was a problem hiding this comment.
I'm not sure I'm a fan of this pattern in Go. I'd rather sacrifice some if-statements to have a more straightforward set of types than have gain an interface and another implementation. I don't feel strongly, so if you like this pattern, 👍.
There was a problem hiding this comment.
I prefer the null object pattern.
There was a problem hiding this comment.
To expand on that, things like zero values, interfaces, and typed nils in Go make me think it's designed for the null object pattern. The added locking cacher interface gives us the option to plug in alternate caching types too.
|
|
||
| lockDir := filepath.Join(config.LocalGitStorageDir, "lfs") | ||
| err := os.MkdirAll(lockDir, 0755) | ||
| func (c *Client) SetupFileCache(path string) error { |
There was a problem hiding this comment.
What do you think about having an option func passed to the constructor instead of this? I would love to avoid having to construct and then call another function before using something. I was thinking:
locking.NewClient(foo, bar, locking.WithFileCache(path))I think this fits the bill of a good optional argument.
There was a problem hiding this comment.
Option funcs are good if the values are needed in the constructor. Otherwise, setting properties seems to be closer to go conventions. You only have to call SetupFileCache() if you want a cacher.
ttaylorr
left a comment
There was a problem hiding this comment.
Thanks for taking a look at my comments. This looks good.
|
Thanks for the review. A |
This updates the locking package to use the new
lfsapi.Clientfrom theapi-masterbranch. As a result, this removes the dependencies on theapiandconfigpackages fromlocking, while addinglfsapiof course.