Merge pull request #646 from great-expectations/cleanup_todos

Update TODOs and a few dosctrings and tests, per notes from reviews on #630 - #634

great_expectations/data_context/data_context.py

@@ -60,7 +60,6 @@
 ALLOWED_DELIMITERS = ['.', '/']
 
 
-#TODO: Maybe find a better name for this guy?
 class ConfigOnlyDataContext(object):
     """This class implements most of the functionality of DataContext, with a few exceptions.
 
@@ -75,6 +74,7 @@ class ConfigOnlyDataContext(object):
     PROFILING_ERROR_CODE_TOO_MANY_DATA_ASSETS = 2
     PROFILING_ERROR_CODE_SPECIFIED_DATA_ASSETS_NOT_FOUND = 3
 
+    # TODO: Consider moving this to DataContext, instead of ConfigOnlyDataContext, since it writes to disc.
     @classmethod
     def create(cls, project_root_dir=None):
         """Build a new great_expectations directory and DataContext object in the provided project_root_dir.
@@ -245,13 +245,11 @@ def expectations_directory(self):
     @property
     def stores(self):
         """A single holder for all Stores in this context"""
-        # TODO: support multiple stores choices and/or ensure abs paths when appropriate
         return self._stores
 
     @property
     def datasources(self):
         """A single holder for all Datasources in this context"""
-        # TODO: support multiple stores choices and/or ensure abs paths when appropriate
         return self._datasources
 
     @property
@@ -1033,7 +1031,8 @@ def get_parameters_in_evaluation_parameter_store_by_run_id(self, run_id):
         else:
             return {}
 
-    #TODO: Can we raname this to _compile_all_evaluation_parameters_from_expectation_suites or something similar?
+    #NOTE: Abe 2019/08/22 : Can we rename this to _compile_all_evaluation_parameters_from_expectation_suites, or something similar?
+    # A more descriptive name would have helped me grok this faster when I first encountered it
     def _compile(self):
         """Compiles all current expectation configurations in this context to be ready for result registration.
         
@@ -1315,7 +1314,7 @@ def update_return_obj(self, data_asset, return_obj):
 
     def build_data_documentation(self, site_names=None, data_asset_name=None):
         """
-        TODO!!!!
+        TODO: Documentation needed
 
         Returns:
             A dictionary with the names of the updated data documentation sites as keys and the the location info

great_expectations/data_context/store/store.py

@@ -14,14 +14,14 @@
 import logging
 logger = logging.getLogger(__name__)
 
-
+# TODO: Rename this to NamespaceAwareStore
 class ContextAwareStore(object):
     def __init__(
         self,
         data_context,
         config,
     ):
-        # FIXME: Eek. This causes circular imports. What to do?
+        # Note: Eek. We want to do type chcecking, but this would cause circular imports. What to do?
         # TODO: remove the dependency. Stores should be based on namespaceIdentifier objects, but not Context itself.
         # if not isinstance(data_context, DataContext):
         #     raise TypeError("data_context must be an instance of type DataContext")

tests/data_context/test_data_context_store_configs.py

@@ -10,7 +10,10 @@
 
 @pytest.fixture(scope="function")
 def totally_empty_data_context(tmp_path_factory):
-    #TODO: This is such a weird workaround for initializing a DataContext. See https://github.com/great-expectations/great_expectations/issues/617
+    # NOTE: This sets up a DataContext with a real path and a config saved to that path.
+    # Now that ConfigOnlyDataContext exists, it's possible to test most DataContext methods without touching the file system.
+    # However, as of 2019/08/22, most tests still use filesystem-based fixtures.
+    # TODO: Where appropriate, switch DataContext tests to the new method.
     project_root_dir = str(tmp_path_factory.mktemp('totally_empty_data_context'))
     os.mkdir(os.path.join(project_root_dir, 'great_expectations'))
 
@@ -42,15 +45,6 @@ def test_create(tmp_path_factory):
     assert isinstance(context, ge.data_context.DataContext)
 
 
-def test_init(tmp_path_factory):
-    #TODO: Deprecating this for now. See https://github.com/great-expectations/great_expectations/issues/617
-
-    # project_path = str(tmp_path_factory.mktemp('path_002'))
-    # context = ge.data_context.DataContext(context_root_dir=project_path)
-
-    # assert isinstance(context, ge.data_context.DataContext)
-    pass
-
 def test_add_store(totally_empty_data_context):
     assert len(totally_empty_data_context.stores.keys()) == 0
 

tests/data_context/test_store.py

@@ -33,7 +33,7 @@
 #     pass
 
 def test_InMemoryStore(empty_data_context):
-    #TODO: Typechecking was causing circular imports. Someday, we can implement this with mypy...?
+    #TODO: Typechecking was causing circular imports. See the note in the Store class.
     # with pytest.raises(TypeError):
     #     my_store = InMemoryStore(
     #         data_context=None,
@@ -241,7 +241,7 @@ def test_NameSpacedFilesystemStore_key_listing(empty_data_context, tmp_path_fact
     assert my_store.get_most_recent_run_id() == "100"
 
 def test_NameSpacedFilesystemStore_pandas_csv_serialization(tmp_path_factory, empty_data_context):
-    #TODO: Generalize this trick, as a way to avoid directory name collisions
+    #TODO: We should consider using this trick everywhere, as a way to avoid directory name collisions
     path = str(tmp_path_factory.mktemp('test_FilesystemStore_pandas_csv_serialization__dir'))
 
     my_store = NameSpacedFilesystemStore(