-
Notifications
You must be signed in to change notification settings - Fork 112
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
Refactor dav #1888
Refactor dav #1888
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@C0rby a couple of minor comments
src := &provider.Reference{Path: srcPath} | ||
dst := &provider.Reference{Path: dstPath} | ||
|
||
intermediateDirRefFunc := func() (*provider.Reference, *rpc.Status, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an overkill. If you want to modularize this, how about just creating a method that takes a ref and returns a ref having the dir, rather than passing the functor object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it looks overkill but for the spaces the way to get the intermediate directory is different. That is why I'm passing the function so that the caller can decide how to get the intermediate dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still prefer an if condition to determine how to get this intermediate directory but we can discuss it in the spaces PR
message: m, | ||
}) | ||
HandleWebdavError(&sublog, w, b, err) | ||
cp := s.prepareCopy(ctx, w, r, srcRef, dstRef, intermediateDirRefFunc, &sublog) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call prepareCopy
from executePathCopy
(maybe rename it)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, executePathCopy is a recursive function and we would need to introduce some condition to only execute prepareCopy once in executePathCopy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also later there will be a executeSpacesCopy
which implements spaces specific stuff, that is why prepareCopy
is a function on its own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for carving the refactoring out of #1803. That will make that PR a lot easier to digest!
I refactored the webdav code to make it cleaner and reusable.
This is a preparation for #1803.
Currently those changes are also in the spaces PR but once this PR is merged we can rebase the spaces PR and reduce its diff.