Repository: altair-viz/altair

Issue:
BUG: Remove invalid methods from top-level chart objects
Examples of methods that never result in valid chart specs:

- ``LayerChart.mark_*``
- ``LayerChart.add_selection``
- ``*ConcatChart.add_selection``
- ``FacetChart.add_selection``
- ``RepeatChart.add_selection``

Discussion:

Patch:
diff --git a/altair/vegalite/v3/api.py b/altair/vegalite/v3/api.py
index f844ef0f..b043680d 100644
--- a/altair/vegalite/v3/api.py
+++ b/altair/vegalite/v3/api.py
@@ -559,18 +559,6 @@ class TopLevelMixin(mixins.ConfigMethodMixin):
                 setattr(copy, key, val)
         return copy
 
-    def add_selection(self, *selections):
-        """Add one or more selections to the chart."""
-        if not selections:
-            return self
-        copy = self.copy(deep=['selection'])
-        if copy.selection is Undefined:
-            copy.selection = {}
-
-        for s in selections:
-            copy.selection[s.name] = s.selection
-        return copy
-
     def project(self, type='mercator', center=Undefined, clipAngle=Undefined, clipExtent=Undefined,
                 coefficient=Undefined, distance=Undefined, fraction=Undefined, lobes=Undefined,
                 parallel=Undefined, precision=Undefined, radius=Undefined, ratio=Undefined,
@@ -1561,6 +1549,18 @@ class Chart(TopLevelMixin, EncodingMixin, mixins.MarkMethodMixin,
         # As a last resort, try using the Root vegalite object
         return core.Root.from_dict(dct, validate)
 
+    def add_selection(self, *selections):
+        """Add one or more selections to the chart."""
+        if not selections:
+            return self
+        copy = self.copy(deep=['selection'])
+        if copy.selection is Undefined:
+            copy.selection = {}
+
+        for s in selections:
+            copy.selection[s.name] = s.selection
+        return copy
+
     def interactive(self, name=None, bind_x=True, bind_y=True):
         """Make chart axes scales interactive
 
@@ -1669,6 +1669,13 @@ class RepeatChart(TopLevelMixin, core.TopLevelRepeatSpec):
         copy.spec = copy.spec.interactive(name=name, bind_x=bind_x, bind_y=bind_y)
         return copy
 
+    def add_selection(self, *selections):
+        """Add one or more selections to the chart."""
+        if not selections or self.spec is Undefined:
+            return self
+        copy = self.copy()
+        copy.spec = copy.spec.add_selection(*selections)
+        return copy
 
 def repeat(repeater='repeat'):
     """Tie a channel to the row or column within a repeated chart
@@ -1712,6 +1719,15 @@ class ConcatChart(TopLevelMixin, core.TopLevelConcatSpec):
         copy |= other
         return copy
 
+    def add_selection(self, *selections):
+        """Add one or more selections to all subcharts."""
+        if not selections or not self.concat:
+            return self
+        copy = self.copy()
+        copy.concat = [chart.add_selection(*selections)
+                       for chart in copy.concat]
+        return copy
+
 
 def concat(*charts, **kwargs):
     """Concatenate charts horizontally"""
@@ -1739,6 +1755,15 @@ class HConcatChart(TopLevelMixin, core.TopLevelHConcatSpec):
         copy |= other
         return copy
 
+    def add_selection(self, *selections):
+        """Add one or more selections to all subcharts."""
+        if not selections or not self.hconcat:
+            return self
+        copy = self.copy()
+        copy.hconcat = [chart.add_selection(*selections)
+                        for chart in copy.hconcat]
+        return copy
+
 
 def hconcat(*charts, **kwargs):
     """Concatenate charts horizontally"""
@@ -1766,6 +1791,15 @@ class VConcatChart(TopLevelMixin, core.TopLevelVConcatSpec):
         copy &= other
         return copy
 
+    def add_selection(self, *selections):
+        """Add one or more selections to all subcharts."""
+        if not selections or not self.vconcat:
+            return self
+        copy = self.copy()
+        copy.vconcat = [chart.add_selection(*selections)
+                        for chart in copy.vconcat]
+        return copy
+
 
 def vconcat(*charts, **kwargs):
     """Concatenate charts vertically"""
@@ -1773,8 +1807,7 @@ def vconcat(*charts, **kwargs):
 
 
 @utils.use_signature(core.TopLevelLayerSpec)
-class LayerChart(TopLevelMixin, EncodingMixin, mixins.MarkMethodMixin,
-                 core.TopLevelLayerSpec):
+class LayerChart(TopLevelMixin, EncodingMixin, core.TopLevelLayerSpec):
     """A Chart with layers within a single panel"""
     def __init__(self, data=Undefined, layer=(), **kwargs):
         # TODO: move common data to top level?
@@ -1829,6 +1862,15 @@ class LayerChart(TopLevelMixin, EncodingMixin, mixins.MarkMethodMixin,
         copy.layer[0] = copy.layer[0].interactive(name=name, bind_x=bind_x, bind_y=bind_y)
         return copy
 
+    def add_selection(self, *selections):
+        """Add one or more selections to all subcharts."""
+        if not selections or not self.layer:
+            return self
+        copy = self.copy()
+        copy.layer = [chart.add_selection(*selections)
+                        for chart in copy.layer]
+        return copy
+
 
 def layer(*charts, **kwargs):
     """layer multiple charts"""
@@ -1865,6 +1907,14 @@ class FacetChart(TopLevelMixin, core.TopLevelFacetSpec):
         copy.spec = copy.spec.interactive(name=name, bind_x=bind_x, bind_y=bind_y)
         return copy
 
+    def add_selection(self, *selections):
+        """Add one or more selections to the chart."""
+        if not selections or self.spec is Undefined:
+            return self
+        copy = self.copy()
+        copy.spec = copy.spec.add_selection(*selections)
+        return copy
+
 
 def topo_feature(url, feature, **kwargs):
     """A convenience function for extracting features from a topojson url


Test patch:
diff --git a/altair/vegalite/v3/tests/test_api.py b/altair/vegalite/v3/tests/test_api.py
index cae64b5e..f0e4b849 100644
--- a/altair/vegalite/v3/tests/test_api.py
+++ b/altair/vegalite/v3/tests/test_api.py
@@ -457,20 +457,6 @@ def test_resolve_methods():
     assert chart.resolve == alt.Resolve(scale=alt.ScaleResolveMap(x='shared', y='independent'))
 
 
-def test_layer_marks():
-    chart = alt.LayerChart().mark_point()
-    assert chart.mark == 'point'
-
-    chart = alt.LayerChart().mark_point(color='red')
-    assert chart.mark == alt.MarkDef('point', color='red')
-
-    chart = alt.LayerChart().mark_bar()
-    assert chart.mark == 'bar'
-
-    chart = alt.LayerChart().mark_bar(color='green')
-    assert chart.mark == alt.MarkDef('bar', color='green')
-
-
 def test_layer_encodings():
     chart = alt.LayerChart().encode(x='column:Q')
     assert chart.encoding.x == alt.X(shorthand='column:Q')
@@ -490,6 +476,34 @@ def test_add_selection():
     assert chart.selection == expected
 
 
+def test_repeat_add_selections():
+    base = alt.Chart('data.csv').mark_point()
+    selection = alt.selection_single()
+    chart1 = base.add_selection(selection).repeat(list('ABC'))
+    chart2 = base.repeat(list('ABC')).add_selection(selection)
+    assert chart1.to_dict() == chart2.to_dict()
+
+
+def test_facet_add_selections():
+    base = alt.Chart('data.csv').mark_point()
+    selection = alt.selection_single()
+    chart1 = base.add_selection(selection).facet('val:Q')
+    chart2 = base.facet('val:Q').add_selection(selection)
+    assert chart1.to_dict() == chart2.to_dict()
+
+
+@pytest.mark.parametrize('charttype', [alt.layer, alt.concat, alt.hconcat, alt.vconcat])
+def test_compound_add_selections(charttype):
+    base = alt.Chart('data.csv').mark_point()
+    selection = alt.selection_single()
+    chart1 = charttype(
+        base.add_selection(selection),
+        base.add_selection(selection)
+    )
+    chart2 = charttype(base, base).add_selection(selection)
+    assert chart1.to_dict() == chart2.to_dict()
+
+
 def test_selection_property():
     sel = alt.selection_interval()
     chart = alt.Chart('data.csv').mark_point().properties(