Added docstrings checks. (#424)

* Added docstrings checks.

* Used google convention.

* Added several docstrings, pep257 check is happy now.

Co-authored-by: chipaca <[email protected]>

ops/framework.py

@@ -208,6 +208,7 @@ def __get__(self, emitter, emitter_type=None):
 
 
 class BoundEvent:
+    """Event bound to an Object."""
 
     def __repr__(self):
         return '<BoundEvent {} bound to {}.{} at {}>'.format(
@@ -380,6 +381,7 @@ def __repr__(self):
 
 
 class PrefixedEvents:
+    """Events to be found in all events using a specific prefix."""
 
     def __init__(self, emitter, key):
         self._emitter = emitter
@@ -390,19 +392,21 @@ def __getattr__(self, name):
 
 
 class PreCommitEvent(EventBase):
-    pass
+    """Events that will be emited first on commit."""
 
 
 class CommitEvent(EventBase):
-    pass
+    """Events that will be emited second on commit."""
 
 
 class FrameworkEvents(ObjectEvents):
+    """Manager of all framework events."""
     pre_commit = EventSource(PreCommitEvent)
     commit = EventSource(CommitEvent)
 
 
 class NoTypeError(Exception):
+    """No class to hold it was found when restoring an event."""
 
     def __init__(self, handle_path):
         self.handle_path = handle_path
@@ -425,6 +429,7 @@ def __str__(self):
 
 
 class Framework(Object):
+    """Main interface to from the Charm to the Operator Framework internals."""
 
     on = FrameworkEvents()
 
@@ -490,6 +495,7 @@ def set_breakpointhook(self):
         return old_breakpointhook
 
     def close(self):
+        """Close the underlying backends."""
         self._storage.close()
 
     def _track(self, obj):
@@ -507,6 +513,7 @@ def _forget(self, obj):
         self._objects.pop(obj.handle.path, None)
 
     def commit(self):
+        """Save changes to the underlying backends."""
         # Give a chance for objects to persist data they want to before a commit is made.
         self.on.pre_commit.emit()
         # Make sure snapshots are saved by instances of StoredStateData. Any possible state
@@ -517,6 +524,7 @@ def commit(self):
         self._storage.commit()
 
     def register_type(self, cls, parent, kind=None):
+        """Register a type to a handle."""
         if parent and not isinstance(parent, Handle):
             parent = parent.handle
         if parent:
@@ -556,6 +564,7 @@ def save_snapshot(self, value):
         self._storage.save_snapshot(value.handle.path, data)
 
     def load_snapshot(self, handle):
+        """Load a persistent snapshot."""
         parent_path = None
         if handle.parent:
             parent_path = handle.parent.path
@@ -571,6 +580,7 @@ def load_snapshot(self, handle):
         return obj
 
     def drop_snapshot(self, handle):
+        """Discard a persistent snapshot."""
         self._storage.drop_snapshot(handle.path)
 
     def observe(self, bound_event: BoundEvent, observer: types.MethodType):
@@ -786,6 +796,7 @@ def remove_unreferenced_events(self):
 
 
 class StoredStateData(Object):
+    """Manager of the stored data."""
 
     def __init__(self, parent, attr_name):
         super().__init__(parent, attr_name)
@@ -803,19 +814,23 @@ def __contains__(self, key):
         return key in self._cache
 
     def snapshot(self):
+        """Return the current state."""
         return self._cache
 
     def restore(self, snapshot):
+        """Restore current state to the given snapshot."""
         self._cache = snapshot
         self.dirty = False
 
     def on_commit(self, event):
+        """Save changes to the storage backend."""
         if self.dirty:
             self.framework.save_snapshot(self)
             self.dirty = False
 
 
 class BoundStoredState:
+    """Stored state data bound to a specific Object."""
 
     def __init__(self, parent, attr_name):
         parent.framework.register_type(StoredStateData, parent)
@@ -954,6 +969,7 @@ def _unwrap_stored(parent_data, value):
 
 
 class StoredDict(collections.abc.MutableMapping):
+    """A dict-like object that uses the StoredState as backend."""
 
     def __init__(self, stored_data, under):
         self._stored_data = stored_data
@@ -986,6 +1002,7 @@ def __eq__(self, other):
 
 
 class StoredList(collections.abc.MutableSequence):
+    """A list-like object that uses the StoredState as backend."""
 
     def __init__(self, stored_data, under):
         self._stored_data = stored_data
@@ -1006,10 +1023,12 @@ def __len__(self):
         return len(self._under)
 
     def insert(self, index, value):
+        """Insert value before index."""
         self._under.insert(index, value)
         self._stored_data.dirty = True
 
     def append(self, value):
+        """Append value to the end of the list."""
         self._under.append(value)
         self._stored_data.dirty = True
 
@@ -1055,16 +1074,25 @@ def __ge__(self, other):
 
 
 class StoredSet(collections.abc.MutableSet):
+    """A set-like object that uses the StoredState as backend."""
 
     def __init__(self, stored_data, under):
         self._stored_data = stored_data
         self._under = under
 
     def add(self, key):
+        """Add a key to a set.
+
+        This has no effect if the key is already present.
+        """
         self._under.add(key)
         self._stored_data.dirty = True
 
     def discard(self, key):
+        """Remove a key from a set if it is a member.
+
+        If the key is not a member, do nothing.
+        """
         self._under.discard(key)
         self._stored_data.dirty = True
 

requirements-dev.txt

@@ -3,3 +3,4 @@
 autopep8
 flake8
 logassert
+pydocstyle

test/test_infra.py

@@ -23,15 +23,19 @@
 from unittest.mock import patch
 
 import autopep8
+import pydocstyle
 from flake8.api.legacy import get_style_guide
 
 import ops
 
 
-def get_python_filepaths():
+def get_python_filepaths(include_tests=True):
     """Helper to retrieve paths of Python files."""
     python_paths = ['setup.py']
-    for root in ['ops', 'test']:
+    roots = ['ops']
+    if include_tests:
+        roots.append('test')
+    for root in roots:
         for dirpath, dirnames, filenames in os.walk(root):
             for filename in filenames:
                 if filename.endswith(".py"):
@@ -73,6 +77,23 @@ def test_pep8(self):
         report += ["\n-- Original flake8 reports:"] + flake8_issues
         self.fail("\n".join(report))
 
+    def test_pep257(self):
+        python_filepaths = get_python_filepaths(include_tests=False)
+        to_ignore = {
+            'D105',  # Missing docstring in magic method
+            'D107',  # Missing docstring in __init__
+        }
+        to_include = pydocstyle.violations.conventions.google - to_ignore
+        errors = list(pydocstyle.check(python_filepaths, select=to_include))
+
+        # if nothing to report, we're done
+        if not errors:
+            return
+
+        report = ["Please fix files as suggested by pydocstyle ({:d} issues):".format(len(errors))]
+        report.extend(str(e) for e in errors)
+        self.fail("\n".join(report))
+
     def test_quote_backslashes(self):
         # ensure we're not using unneeded backslash to escape strings
         issues = []