remote: separate tree functions from main remote classes #3931
Conversation
pmrowla
force-pushed
the
remote-tree-initial
branch
from
ebb1e61 to
71f4540
Compare
pmrowla
changed the title
| @@ -65,14 +99,7 @@ def get_etag(self, path_info): | |||
| def get_file_checksum(self, path_info): | |||
| return self.get_etag(path_info) | |||
There was a problem hiding this comment.
Should etag be part of the tree too? Kinda like stat.
| self.blob_service.delete_blob(path_info.bucket, path_info.path) | ||
|
|
||
| def _list_paths(self, bucket, prefix, progress_callback=None): | ||
| def list_paths(self, bucket, prefix, progress_callback=None): |
There was a problem hiding this comment.
It probably should be. I think it makes sense to implement walk_files for the remotes which still do not have it as a part of this overall refactoring, but in this PR I was only looking to start moving functions into the tree classes without changing existing behavior and potentially breaking things.
| class GSRemoteTree(BaseRemoteTree): | ||
| @property | ||
| def gs(self): | ||
| return self.remote.gs |
There was a problem hiding this comment.
This is kinda strange. Shouldn't the connection be part of the Tree?
There was a problem hiding this comment.
Or is it just a convenience that will be fixed after the transition?
There was a problem hiding this comment.
I think all of the connection/config related code for all of the remotes needs to be moved into the trees. This (and the equivalent properties in the other remotes) is just a temporary fix since I was trying to avoid moving everything all at once
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.
❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)
Thank you for the contribution - we'll try to review it as soon as possible. 🙏
This PR does not change any existing behavior, it only moves an initial set of functions from
RemoteintoRemoteTreeclasses (which can be accessed viaremote.tree). The next set of functions to be moved for this refactor (separating remote and cache behavior) will also need to accept explicit tree parameters, so we need some minimal RemoteTree functionality before that.upload()anddownload()will be moved into the trees in a future PR.Related to #3882.