Fix some reported bugs + improve error handling

spyder/plugins/vcs/utils/api.py

@@ -185,7 +185,7 @@ class VCSBackendBase(object):
     **Error handling**
 
     By default, if a generic error occurred,
-    all the method for VCS operations raises a :class:`~VCSUnexpectedError`
+    all the method for VCS operations raises a :class:`~VCSFeatureError`
     and all the properties raises :class:`~VCSPropertyError`.
 
     If any requisite stopped working
@@ -443,6 +443,9 @@ def __init__(self, repodir: str):
 
     def __init_subclass__(cls, **kwargs):
         """Adjust subclass attributes to match the feature name."""
+        def _dummy_feature(self, *_, **__):
+            raise NotImplementedError("This feature is not implemented yet.")
+
         super().__init_subclass__(**kwargs)
         attrs = {
             key: getattr(cls, key)
@@ -457,6 +460,21 @@ def __init_subclass__(cls, **kwargs):
                     # if the attribute already exists.
                     setattr(cls, attr.__name__, attr)
 
+            # check if attr is a property.
+            elif isinstance(attr, property):
+                # check if property is a feature
+                is_feature = getattr(attr.fget, "_is_feature", None)
+                if is_feature:
+                    # Add disabled feature for fget and fdel if they does not exists.
+                    if attr.fset is None:
+                        attr.setter(feature(name=key, enabled=False)(_dummy_feature))
+                    elif not getattr(attr.fset, "_is_feature", None):
+                        attr.setter(feature(name=key, enabled=False)(attr.fset))
+
+                    if attr.fdel is None:
+                        attr.deleter(feature(name=key, enabled=False)(_dummy_feature))
+                    elif not getattr(attr.fdel, "_is_feature", None):
+                        attr.deleter(feature(name=key, enabled=False)(attr.fdel))
     @property
     def type(self) -> type:
         """

spyder/plugins/vcs/utils/backend.py

@@ -30,7 +30,7 @@
 
 from .api import VCSBackendBase, ChangedStatus, feature
 from .errors import (VCSAuthError, VCSPropertyError, VCSBackendFail,
-                     VCSUnexpectedError)
+                     VCSFeatureError)
 from .mixins import CredentialsKeyringMixin
 
 __all__ = ("GitBackend", "MercurialBackend")
@@ -258,8 +258,8 @@ def editable_branches(self) -> list:
     def create_branch(self, branchname: str, empty: bool = False) -> bool:
         git = programs.find_program("git")
         if branchname in self.branches:
-            raise VCSUnexpectedError(
-                method="create_branch",
+            raise VCSFeatureError(
+                feature=self.create_branch,
                 error="Failed to create branch {}"
                 "since it already exists".format(branchname),
             )
@@ -299,8 +299,8 @@ def change(self,
                prefer_unstaged: bool = False) -> typing.Dict[str, object]:
         filestates = get_git_status(self.repodir, path, nobranch=True)[2]
         if filestates is None:
-            raise VCSUnexpectedError(
-                method="change",
+            raise VCSFeatureError(
+                feature=self.change,
                 error="Failed to get git changes",
             )
 
@@ -324,7 +324,7 @@ def stage(self, path: str) -> bool:
 
     @feature()
     def unstage(self, path: str) -> bool:
-        retcode = self._run(["reset", "--", _escape_path(path)])[0]
+        retcode, _, err = self._run(["reset", "--", _escape_path(path)])
         if retcode == 0:
             change = self.change(path, prefer_unstaged=False)
             if change and not change["staged"]:
@@ -401,8 +401,8 @@ def undo_commit(
         )
 
         if retcode:
-            raise VCSUnexpectedError(
-                "undo_commit",
+            raise VCSFeatureError(
+                feature=self.undo_commit,
                 error="Failed get the number of commits in branch {}".format(
                     self.branch),
                 raw_error=err.decode(),
@@ -430,8 +430,8 @@ def undo_commit(
             )
 
             if retcode:
-                # raise VCSUnexpectedError(
-                #     "get_last_commits",
+                # raise VCSFeatureError(
+                #     feature=self.get_last_commits,
                 #     error="Failed to get git history",
                 #     raw_error=err.decode(),
                 # )
@@ -443,8 +443,8 @@ def undo_commit(
             ["reset", "--soft", "HEAD~" + str(commits)], git=git)
 
         if retcode:
-            raise VCSUnexpectedError(
-                "get_last_commits",
+            raise VCSFeatureError(
+                feature=self.get_last_commits,
                 error="Failed to undo {} commits".format(commits),
                 raw_error=err.decode(),
             )
@@ -490,8 +490,8 @@ def get_last_commits(
             "description:%n%b%x00",
         ])
         if retcode != 0:
-            raise VCSUnexpectedError(
-                method="get_last_commits",
+            raise VCSFeatureError(
+                feature=self.get_last_commits,
                 error="Failed to get git history",
                 raw_error=err.decode(),
             )
@@ -600,14 +600,17 @@ def _remote_operation(self, operation: str, *args):
         if status is False:
             # Auth failed
             raise VCSAuthError(
+                credentials={
+                    "username": username,
+                    "password": credentials.get("password"),
+                },
+                credentials_callback=lambda x: setattr(self, "credentials", x),
                 required_credentials=self.REQUIRED_CREDENTIALS,
-                username=username,
-                password=credentials.get("password"),
                 error="Wrong credentials",
             )
 
-        raise VCSUnexpectedError(
-            method=operation,
+        raise VCSFeatureError(
+            method=getattr(self, operation),
             error="Failed to {} from remote".format(operation),
         )
 
@@ -650,6 +653,11 @@ def branch(self) -> str:
             return revision[2]
         raise VCSPropertyError("branch", "get")
 
+    @branch.setter
+    @feature(enabled=False)
+    def branch(self, branch: str) -> None:
+        pass
+
 
 # --- VCS operation functions ---
 

spyder/plugins/vcs/utils/errors.py

@@ -42,32 +42,36 @@ def __init__(self,
         self.raw_error = raw_error
 
 
-class VCSUnexpectedError(VCSError):
+class VCSFeatureError(VCSError):
     """
-    Raised when a bad error is occurred.
+    Raised when a generic error happens in the feature.
+
+    Currently, only non-property features can raise this error.
 
     Parameters
     ----------
-    method : str, optional
-        The method where the error occurred. The default is None.
-
-    Notes
-    -----
-    This error is never raised by properties.
+    feature : Callable[..., object]
+        The feature where the error occurred.
+        Must be a valid feature decorated with :func:`~.api.feature
 
     See Also
     --------
     VCSPropertyError
     """
 
-    __slots__ = ("method", )
+    __slots__ = ("feature", )
 
-    def __init__(self,
-                 *args: object,
-                 method: typing.Optional[str] = None,
+    def __init__(self, *args: object, feature: typing.Callable[..., object],
                  **kwargs: typing.Optional[str]):
         super().__init__(*args, **kwargs)
-        self.method = method
+        self.feature = feature
+
+    def retry(self, *args, **kwargs) -> object:
+        """Call again the feature with given parameters."""
+        if self.feature.enabled:
+            return self.feature(*args, **kwargs)
+        raise NotImplementedError("The feature {} is disabled.".format(
+            self.feature.__name__))
 
 
 class VCSPropertyError(VCSError):
@@ -89,7 +93,7 @@ class VCSPropertyError(VCSError):
 
     See Also
     --------
-    VCSUnexpectedError
+    VCSFeatureError
     """
 
     __slots__ = ("name", "operation")
@@ -120,13 +124,14 @@ class VCSBackendFail(VCSError):
         A list of missing executables. The default is an empty list.
     modules : list, optional
         A list of missing python modules. The default is an empty list.
-    is_valid_repository : bool, optional
-        A flag indicating if the directory contains a valid repository.
-        The default is True.
 
         .. note::
             Module does refer to actual import-style module name,
             not pip package name.
+
+    is_valid_repository : bool, optional
+        A flag indicating if the directory contains a valid repository.
+        The default is True.
     """
     __slots__ = ("directory", "backend_type", "programs", "modules",
                  "is_valid_repository")
@@ -159,40 +164,58 @@ class VCSAuthError(VCSError):
 
     Parameters
     ----------
-    username : str, optional
-        The set username. The default is None.
-    password : str, optional
-        The set password. The default is None.
-    email : str, optional
-        The set email. The default is None.
-    token : str, optional
-        The set token. The default is None.
-    required_credentials : str, optional
+    required_credentials : str
         The credentials that the backend requires.
+    credentials_callback : str
+        The function to call when :meth:`VCSAuthError.set_credentials`
+        is called.
+    credentials : Dict[str, object], optional
+        The credentials that causes the error.
+
     """
 
-    __slots__ = ("username", "password", "email", "token",
-                 "required_credentials")
+    __slots__ = ("required_credentials", "_credentials",
+                 "_credentials_callback")
 
     def __init__(self,
                  required_credentials: typing.Sequence[str],
+                 credentials_callback: typing.Callable[..., None],
                  *args: object,
-                 username: typing.Optional[str] = None,
-                 password: typing.Optional[str] = None,
-                 email: typing.Optional[str] = None,
-                 token: typing.Optional[str] = None,
+                 credentials: typing.Optional[typing.Dict[str, object]] = None,
                  **kwargs: typing.Optional[str]):
 
         super().__init__(*args, **kwargs)
         self.required_credentials = required_credentials
-        self.username = username
-        self.password = password
-        self.email = email
-        self.token = token
+        self._credentials = credentials
+        self._credentials_callback = credentials_callback
 
     @property
-    def are_credentials_inserted(self) -> bool:
-        """Check if the required credentials was inserted."""
-        return all(
-            getattr(self, key, None) is None
-            for key in self.required_credentials)
+    def credentials(self) -> typing.Dict[str, object]:
+        """
+        A proxy for required backend credentials.
+
+        This property contains the required credentials fields.
+
+        You can set to it update the backend credentials.
+
+        It is preferred to interact with this property instead of
+        using the :attr:`.VCSBackendBase.credentials` property directly.
+
+        Raises
+        ------
+        ValueError
+            When setting, if one of the required field is missing.
+        """
+        return {
+            key: self._credentials.get(key)
+            for key in self.required_credentials
+        }
+
+    @credentials.setter
+    def credentials(self, credentials: typing.Dict[str, object]) -> None:
+        new_cred = {}
+        for key in self.required_credentials:
+            if credentials.get(key) is None:
+                raise ValueError("Missing field {}".format(key))
+            new_cred[key] = credentials[key]
+        self._credentials_callback(new_cred)

spyder/plugins/vcs/utils/mixins.py

@@ -83,8 +83,9 @@ def credentials(self, credentials: typing.Dict[str, object]) -> None:
             i = self._get_last_token()
             if i == -1:
                 raise VCSAuthError(
+                    credentials={"token", None},
+                    credentials_callback=lambda x: setattr(self, "credentials", x),
                     required_credentials=self.REQUIRED_CREDENTIALS,
-                    token=None,
                     error="No token is found for {}".format(
                         self.credential_context),
                 )
@@ -129,7 +130,8 @@ def credentials(self, credentials: typing.Dict[str, object]) -> None:
                     }
                 else:
                     raise VCSAuthError(
-                        **{type_: user},
+                        credentials={type_: user},
+                        credentials_callback=lambda x: setattr(self, "credentials", x),
                         required_credentials=self.REQUIRED_CREDENTIALS,
                         error="The given {} is not found for {}".format(
                             type_, self.credential_context))

spyder/plugins/vcs/widgets/common.py

@@ -70,7 +70,7 @@ def __init__(self, manager: VCSBackendManager, *args: object,
         self._manager = manager
         self.setEditable(True)
         # branches cache
-        self._branches: typing.List[str] = []
+        self._branches: typing.Set[str] = set()
 
         self.sig_branch_changed.connect(self.refresh)
 
@@ -79,20 +79,20 @@ def refresh(self) -> None:
         """Clear and re-add branches."""
         def _task():
             """Task executed in another thread."""
-            branches = []
+            branches = set()
             if manager.type.branch.fget.enabled:
                 current_branch = manager.branch
             else:
                 current_branch = None
 
             if manager.type.editable_branches.fget.enabled:
                 try:
-                    branches.extend(manager.editable_branches)
+                    branches.update(manager.editable_branches)
                 except VCSPropertyError:
                     # Suppress editable_branches fail
                     pass
             elif current_branch is not None:
-                branches.append(manager.branch)
+                branches.add(manager.branch)
             return (current_branch, branches)
 
         @Slot(object)
@@ -101,8 +101,8 @@ def _handle_result(result):
                 current_branch, self._branches = result
                 # Block signals to reduce useless signal emissions
                 oldstate = self.blockSignals(True)
-
-                self.addItems(self._branches)
+                self.clear()
+                self.addItems(tuple(self._branches))
                 if current_branch is not None:
                     index = self.findText(current_branch)
                     if index == -1: