[ENHANCEMENT] DataContext.clean_data_docs now raises helpful errors

docs/changelog.rst

@@ -10,6 +10,7 @@ Develop
 * [ENHANCEMENT] CLI `docs build` command implemented for v3 api
 * [ENHANCEMENT] CLI `docs clean` command implemented for v3 api
 * [MAINTENANCE] Add testing for overwrite_existing in sanitize_yaml_and_save_datasource #2613
+* [ENHANCEMENT] DataContext.clean_data_docs now raises helpful errors
 
 0.13.15
 -----------------

great_expectations/cli/docs.py

@@ -1,15 +1,10 @@
-import sys
-
 import click
 
 from great_expectations import DataContext
 from great_expectations.cli import toolkit
 from great_expectations.cli.build_docs import build_docs
-from great_expectations.cli.pretty_printing import (
-    cli_message,
-    cli_message_list,
-    display_not_implemented_message_and_exit,
-)
+from great_expectations.cli.pretty_printing import cli_message, cli_message_list
+from great_expectations.exceptions import DataContextError
 
 
 @click.group()
@@ -93,18 +88,26 @@ def clean_data_docs(ctx, site_name=None, all_sites=False):
     """Delete data docs"""
     context = ctx.obj.data_context
 
-    if site_name is None and all_sites is False:
+    if (site_name is None and all_sites is False) or (site_name and all_sites):
         toolkit.exit_with_failure_message_and_stats(
             context,
             usage_event="cli.docs.clean",
-            message="<red>Please specify --all to remove all sites or specify a specific site using --site_name</red>",
+            message="<red>Please specify either --all to remove all sites or a specific site using --site_name</red>",
+        )
+    try:
+        # if site_name is None, context.clean_data_docs(site_name=site_name)
+        # will clean all sites.
+        context.clean_data_docs(site_name=site_name)
+        toolkit.send_usage_message(
+            data_context=context, event="cli.docs.clean", success=True
+        )
+        cli_message("<green>{}</green>".format("Cleaned data docs"))
+    except DataContextError as de:
+        toolkit.exit_with_failure_message_and_stats(
+            context,
+            usage_event="cli.docs.clean",
+            message=f"<red>{de}</red>",
         )
-    # if site_name is None, context.clean_data_docs(site_name=site_name) will clean all sites.
-    context.clean_data_docs(site_name=site_name)
-    toolkit.send_usage_message(
-        data_context=context, event="cli.docs.clean", success=True
-    )
-    cli_message("<green>{}</green>".format("Cleaned data docs"))
 
 
 def _build_intro_string(docs_sites_strings):

great_expectations/data_context/data_context.py

@@ -2313,39 +2313,53 @@ def build_data_docs(
 
         return index_page_locator_infos
 
-    def clean_data_docs(self, site_name=None):
-        sites123 = self.project_config_with_variables_substituted.data_docs_sites
-        cleaned = False
-        for sname, site_config in sites123.items():
-            if site_name is None:
-                cleaned = False
-                complete_site_config = site_config
-                module_name = "great_expectations.render.renderer.site_builder"
-                site_builder = instantiate_class_from_config(
-                    config=complete_site_config,
-                    runtime_environment={
-                        "data_context": self,
-                        "root_directory": self.root_directory,
-                    },
-                    config_defaults={"module_name": module_name},
+    def clean_data_docs(self, site_name=None) -> bool:
+        """
+        Clean a given data docs site.
+
+        This removes all files from the configured Store.
+
+        Args:
+            site_name (str): Optional, the name of the site to clean. If not
+            specified, all sites will be cleaned.
+        """
+        data_docs_sites = self.project_config_with_variables_substituted.data_docs_sites
+        if not data_docs_sites:
+            raise ge_exceptions.DataContextError(
+                "No data docs sites were found on this DataContext, therefore no sites will be cleaned.",
+            )
+
+        data_docs_site_names = list(data_docs_sites.keys())
+        if site_name:
+            if site_name not in data_docs_site_names:
+                raise ge_exceptions.DataContextError(
+                    f"The specified site name `{site_name}` does not exist in this project."
                 )
-                site_builder.clean_site()
-                cleaned = True
-            else:
-                if site_name == sname:
-                    complete_site_config = site_config
-                    module_name = "great_expectations.render.renderer.site_builder"
-                    site_builder = instantiate_class_from_config(
-                        config=complete_site_config,
-                        runtime_environment={
-                            "data_context": self,
-                            "root_directory": self.root_directory,
-                        },
-                        config_defaults={"module_name": module_name},
-                    )
-                    site_builder.clean_site()
-                    return True
-        return cleaned
+            return self._clean_data_docs_site(site_name)
+
+        cleaned = []
+        for existing_site_name in data_docs_site_names:
+            cleaned.append(self._clean_data_docs_site(existing_site_name))
+        return all(cleaned)
+
+    def _clean_data_docs_site(self, site_name: str) -> bool:
+        sites = self.project_config_with_variables_substituted.data_docs_sites
+        if not sites:
+            return False
+        site_config = sites.get(site_name)
+
+        site_builder = instantiate_class_from_config(
+            config=site_config,
+            runtime_environment={
+                "data_context": self,
+                "root_directory": self.root_directory,
+            },
+            config_defaults={
+                "module_name": "great_expectations.render.renderer.site_builder"
+            },
+        )
+        site_builder.clean_site()
+        return True
 
     def profile_datasource(
         self,

tests/cli/test_docs.py

@@ -315,26 +315,45 @@ def context_with_site_built(titanic_data_context_stats_enabled_config_version_3)
     return context
 
 
+@pytest.mark.parametrize(
+    "invocation,expected_error",
+    [
+        (
+            "--v3-api docs clean",
+            "Please specify either --all to remove all sites or a specific site using --site_name",
+        ),
+        (
+            "--v3-api docs clean --site-name local_site --all",
+            "Please specify either --all to remove all sites or a specific site using --site_name",
+        ),
+        (
+            "--v3-api docs clean --site-name fake_site",
+            "The specified site name `fake_site` does not exist in this project.",
+        ),
+    ],
+)
 @mock.patch(
     "great_expectations.core.usage_statistics.usage_statistics.UsageStatisticsHandler.emit"
 )
-def test_docs_clean_no_site_specified_raises_helpful_error(
-    mock_emit, caplog, monkeypatch, context_with_site_built
+def test_docs_clean_raises_helpful_errors(
+    mock_emit, invocation, expected_error, caplog, monkeypatch, context_with_site_built
 ):
+    """
+    Test that helpful error messages are raised when:
+        - invalid combinations of the --all and --site-name flags are used
+        - invalid site names are specified
+    """
     context = context_with_site_built
     runner = CliRunner(mix_stderr=True)
     monkeypatch.chdir(os.path.dirname(context.root_directory))
     result = runner.invoke(
         cli,
-        "--v3-api docs clean",
+        invocation,
         catch_exceptions=False,
     )
     stdout = result.stdout
     assert result.exit_code == 1
-    assert (
-        "Please specify --all to remove all sites or specify a specific site using --site_name"
-        in stdout
-    )
+    assert expected_error in stdout
     assert "Cleaned data docs" not in stdout
     assert mock_emit.call_count == 2
     assert mock_emit.call_args_list == [

tests/data_context/test_data_context_data_docs_api.py

@@ -1,3 +1,4 @@
+import os
 from unittest import mock
 
 import pytest
@@ -43,7 +44,7 @@ def test_open_docs_with_single_local_site(mock_webbrowser, empty_data_context):
 
 
 @pytest.fixture
-def context_with_multiple_sites(empty_data_context):
+def context_with_multiple_built_sites(empty_data_context):
     context = empty_data_context
     config = context.project_config_with_variables_substituted
     multi_sites = {
@@ -66,6 +67,7 @@ def context_with_multiple_sites(empty_data_context):
     }
     config.data_docs_sites = multi_sites
     context._project_config = config
+    context.build_data_docs()
     obs = context.get_docs_sites_urls(only_if_exists=False)
     assert len(obs) == 2
     assert obs[0]["site_url"].endswith(
@@ -77,6 +79,16 @@ def context_with_multiple_sites(empty_data_context):
         "great_expectations/uncommitted/data_docs/another_local_site/index.html"
     )
     assert obs[1]["site_name"] == "another_local_site"
+    for site in ["local_site", "another_local_site"]:
+        assert os.path.isfile(
+            os.path.join(
+                context.root_directory,
+                context.GE_UNCOMMITTED_DIR,
+                "data_docs",
+                site,
+                "index.html",
+            )
+        )
 
     return context
 
@@ -129,8 +141,10 @@ def context_with_multiple_local_sites_and_s3_site(empty_data_context):
 
 
 @mock.patch("webbrowser.open", return_value=True, side_effect=None)
-def test_open_docs_with_two_local_sites(mock_webbrowser, context_with_multiple_sites):
-    context = context_with_multiple_sites
+def test_open_docs_with_two_local_sites(
+    mock_webbrowser, context_with_multiple_built_sites
+):
+    context = context_with_multiple_built_sites
     context.open_data_docs(only_if_exists=False)
     assert mock_webbrowser.call_count == 2
     first_call = mock_webbrowser.call_args_list[0][0][0]
@@ -147,9 +161,9 @@ def test_open_docs_with_two_local_sites(mock_webbrowser, context_with_multiple_s
 
 @mock.patch("webbrowser.open", return_value=True, side_effect=None)
 def test_open_docs_with_two_local_sites_specify_open_one(
-    mock_webbrowser, context_with_multiple_sites
+    mock_webbrowser, context_with_multiple_built_sites
 ):
-    context = context_with_multiple_sites
+    context = context_with_multiple_built_sites
     context.open_data_docs(site_name="another_local_site", only_if_exists=False)
 
     assert mock_webbrowser.call_count == 1
@@ -176,17 +190,17 @@ def test_get_docs_sites_urls_with_no_sites_specify_one(context_with_no_sites):
 
 
 def test_get_docs_sites_urls_with_non_existent_site_raises_error(
-    context_with_multiple_sites,
+    context_with_multiple_built_sites,
 ):
-    context = context_with_multiple_sites
+    context = context_with_multiple_built_sites
     with pytest.raises(DataContextError):
         context.get_docs_sites_urls(site_name="not_a_real_site")
 
 
 def test_get_docs_sites_urls_with_two_local_sites_specify_one(
-    context_with_multiple_sites,
+    context_with_multiple_built_sites,
 ):
-    context = context_with_multiple_sites
+    context = context_with_multiple_built_sites
     obs = context.get_docs_sites_urls(
         site_name="another_local_site", only_if_exists=False
     )
@@ -198,3 +212,50 @@ def test_get_docs_sites_urls_with_two_local_sites_specify_one(
     assert url.endswith(
         "/great_expectations/uncommitted/data_docs/another_local_site/index.html"
     )
+
+
+def test_clean_data_docs_on_context_with_no_sites_raises_error(
+    context_with_no_sites,
+):
+    context = context_with_no_sites
+    with pytest.raises(DataContextError):
+        context.clean_data_docs()
+
+
+def test_clean_data_docs_on_context_with_multiple_sites_with_no_site_name_cleans_all_sites_and_returns_true(
+    context_with_multiple_built_sites,
+):
+    context = context_with_multiple_built_sites
+    assert context.clean_data_docs() is True
+    for site in ["local_site", "another_local_site"]:
+        assert not os.path.isfile(
+            os.path.join(
+                context.root_directory,
+                context.GE_UNCOMMITTED_DIR,
+                "data_docs",
+                site,
+                "index.html",
+            )
+        )
+
+
+def test_clean_data_docs_on_context_with_multiple_sites_with_existing_site_name_cleans_selected_site_and_returns_true(
+    context_with_multiple_built_sites,
+):
+    context = context_with_multiple_built_sites
+    assert context.clean_data_docs(site_name="another_local_site") is True
+    data_docs_dir = os.path.join(
+        context.root_directory, context.GE_UNCOMMITTED_DIR, "data_docs"
+    )
+    assert not os.path.isfile(
+        os.path.join(data_docs_dir, "another_local_site", "index.html")
+    )
+    assert os.path.isfile(os.path.join(data_docs_dir, "local_site", "index.html"))
+
+
+def test_clean_data_docs_on_context_with_multiple_sites_with_non_existent_site_name_raises_error(
+    context_with_multiple_built_sites,
+):
+    context = context_with_multiple_built_sites
+    with pytest.raises(DataContextError):
+        assert context.clean_data_docs(site_name="not_a_real_site")