Update docstring, comments and todos

- Intermediate commit. Please rebase or ammend.
- Discuss the todos and comments, fix issues if
  any. Update docstring, comments and remove
  todos.
- Need travis ci

Author: cslzchen

waterbutler/core/metadata.py

@@ -136,10 +136,10 @@ def kind(self) -> str:
     @property
     @abc.abstractmethod
     def id(self) -> str:
-        """
-        This property consolidated "path-based" ids and "id-based" ids under a single property. This
-        makes inter provider move/copies easier. This should be identical to the path object
-        identifier as this is used to revalidate paths.
+        """ This property consolidates path-based and id-based file/folder/object identity under a
+        single one: ``id``, which makes inter provider move/copy actions easier.  For path-based
+        providers, the value is identical to ``path`` property.  For id-based providers, it is the
+        raw identity by definition (``self.raw['id']`` for most providers).
         """
         raise NotImplementedError
 

waterbutler/providers/bitbucket/metadata.py

@@ -24,14 +24,15 @@ def __init__(self, raw, path_obj, owner=None, repo=None):
         self.owner = owner
         self.repo = repo
 
-    @property
-    def id(self):
-        return self.build_path()
-
     @property
     def provider(self):
         return 'bitbucket'
 
+    # Bitbucket is a path-based provider and uses the default ``revalidate_path()``.
+    @property
+    def id(self):
+        return self.path
+
     @property
     def path(self):
         return self.build_path()

waterbutler/providers/box/metadata.py

@@ -7,14 +7,15 @@ def __init__(self, raw, path_obj):
         super().__init__(raw)
         self._path_obj = path_obj
 
-    @property
-    def id(self):
-        return self.raw['id']
-
     @property
     def provider(self):
         return 'box'
 
+    # Box is an id-based provider and uses its own ``revalidate_path()``.
+    @property
+    def id(self):
+        return self.raw['id']
+
     @property
     def materialized_path(self):
         return str(self._path_obj)

waterbutler/providers/cloudfiles/metadata.py

@@ -5,14 +5,18 @@
 
 class BaseCloudFilesMetadata(metadata.BaseMetadata):
 
-    @property
-    def id(self):
-        return self.path
-
     @property
     def provider(self):
         return 'cloudfiles'
 
+    # CloudFiles is a path-based provider.  However, it never called ``revalidate_path()`` even when
+    # it was the backend storage provider for OSFStorage.  The provider is no longer active until it
+    # is upgraded to an addon provider for the users.
+    @property
+    def id(self):
+        # TODO: Do we need this ``id`` to be actually set?
+        return self.path
+
 
 class CloudFilesFileMetadata(BaseCloudFilesMetadata, metadata.BaseFileMetadata):
 

waterbutler/providers/dataverse/metadata.py

@@ -3,14 +3,18 @@
 
 class BaseDataverseMetadata(metadata.BaseMetadata):
 
-    @property
-    def id(self):
-        return str(self.raw['id'])
-
     @property
     def provider(self):
         return 'dataverse'
 
+    # TODO: Should this be in ``DataverseFileMetadata``?
+    # TODO: Does ``self.raw['id']`` exists for ``DataverseDatasetMetadata``?
+    # TODO: DataVerse's ``revalidate_path()`` looks very different. Is this an issue?
+    # DataVerse is an id-based provider and has its own ``revalidate_path()``.
+    @property
+    def id(self):
+        return str(self.raw['id'])
+
 
 class DataverseFileMetadata(BaseDataverseMetadata, metadata.BaseFileMetadata):
 

waterbutler/providers/dropbox/metadata.py

@@ -9,14 +9,16 @@ def __init__(self, raw, folder):
         super().__init__(raw)
         self._folder = folder
 
-    @property
-    def id(self):
-        return self.raw['id']
-
     @property
     def provider(self):
         return 'dropbox'
 
+    # TODO: Is Dropbox path-based instead of id-based?
+    # TODO: Dropbox does not have a ``revalidate_path()``, should we use path instead of id?
+    @property
+    def id(self):
+        return self.raw['id']
+
     def build_path(self, path):
         # TODO write a test for this
         if path.lower().startswith(self._folder.lower()):

waterbutler/providers/figshare/metadata.py

@@ -20,6 +20,7 @@ def __init__(self, raw, raw_file=None):
         else:
             self.raw_file = self.raw['files'][0]
 
+    # FigShare are id-based providers and use their own ``revalidate_path()``.
     @property
     def id(self):
         return self.raw_file['id']
@@ -116,6 +117,7 @@ class FigshareFolderMetadata(BaseFigshareMetadata, metadata.BaseFolderMetadata):
     considered folders.
     """
 
+    # FigShare are id-based providers
     @property
     def id(self):
         return self.raw['id']

waterbutler/providers/filesystem/metadata.py

@@ -9,14 +9,15 @@ def __init__(self, raw, folder):
         super().__init__(raw)
         self._folder = folder
 
-    @property
-    def id(self):
-        return self.build_path(self.raw['path'])
-
     @property
     def provider(self):
         return 'filesystem'
 
+    # FileSystem is a path-based provider and uses the default ``revalidate_path()``.
+    @property
+    def id(self):
+        return self.path
+
     def build_path(self, path):
         # TODO write a test for this
         if path.lower().startswith(self._folder.lower()):

waterbutler/providers/github/metadata.py

@@ -19,14 +19,16 @@ def __init__(self, raw, commit=None, ref=None):
         self.commit = commit
         self.ref = ref
 
-    @property
-    def id(self):
-        return self.path
-
     @property
     def provider(self):
         return 'github'
 
+    # GitHub is a path-based provider. However, it has its own ``revalidate_path()``.
+    # TODO: Investigate if this is an issue.
+    @property
+    def id(self):
+        return self.path
+
     @property
     def extra(self):
         ret = {}

waterbutler/providers/gitlab/metadata.py

@@ -15,14 +15,15 @@ def __init__(self, raw, path):
         super().__init__(raw)
         self._path_obj = path
 
-    @property
-    def id(self) -> str:
-        return self.build_path()
-
     @property
     def provider(self) -> str:
         return 'gitlab'
 
+    # GitLab is a path-based provider and uses the default ``revalidate_path()``.
+    @property
+    def id(self) -> str:
+        return self.path
+
     @property
     def path(self) -> str:
         return self.build_path()