Remove the default of collections to autodiscovery

See #328

CHANGELOG.rst

@@ -9,6 +9,12 @@ Package maintainers and users who have to manually update their installation
 may want to subscribe to `GitHub's tag feed
 <https://github.com/untitaker/vdirsyncer/tags.atom>`_.
 
+Version 0.9.0
+=============
+
+- The ``collections`` parameter is now required in pair configurations.
+  Vdirsyncer will tell you what to do in its error message. See :gh:`328`.
+
 Version 0.8.1
 =============
 

config.example

@@ -19,16 +19,11 @@ status_path = ~/.vdirsyncer/status/
 a = bob_contacts_local
 b = bob_contacts_remote
 
-
-# Synchronize all collections available on "side B" (in this case the server).
+# Synchronize all collections that can be found.
 # You need to run `vdirsyncer discover` if new calendars/addressbooks are added
 # on the server.
 
-# Omitting this parameter implies that the given path and URL in the
-# corresponding `[storage <name>]` blocks are already directly pointing to a
-# collection each.
-
-collections = ["from b"]
+collections = ["from a", "from b"]
 
 # Synchronize the "display name" property into a local file (~/.contacts/displayname).
 metadata = ["displayname"]
@@ -58,7 +53,7 @@ url = https://owncloud.example.com/remote.php/carddav/
 [pair bob_calendar]
 a = bob_calendar_local
 b = bob_calendar_remote
-collections = ["private", "work"]
+collections = ["from a", "from b"]
 
 # Calendars also have a color property
 metadata = ["displayname", "color"]

docs/config.rst

@@ -52,14 +52,11 @@ Pair Section
 
 - ``a`` and ``b`` reference the storages to sync by their names.
 
-- ``collections``: Optional, a list of collections to synchronize when
-  ``vdirsyncer sync`` is executed. If this parameter is omitted, it is assumed
-  the storages are already directly pointing to one collection each. Specifying
-  a collection multiple times won't make vdirsyncer sync that collection more
-  than once.
+- ``collections``: A list of collections to synchronize when
+  ``vdirsyncer sync`` is executed.
 
-  Furthermore, there are the special values ``"from a"`` and ``"from b"``,
-  which tell vdirsyncer to try autodiscovery on a specific storage.
+  The special values ``"from a"`` and ``"from b"``, tell vdirsyncer to try
+  autodiscovery on a specific storage.
 
   Examples:
 

docs/tutorial.rst

@@ -72,6 +72,7 @@ default addressbook to ``~/.contacts/``::
     [pair my_contacts]
     a = my_contacts_local
     b = my_contacts_remote
+    collections = null
 
     [storage my_contacts_local]
     type = filesystem
@@ -122,14 +123,14 @@ value.
 Collection discovery
 --------------------
 
-Configuring each collection (=addressbook/calendar) becomes extremely
-repetitive if they are all on the same server. Vdirsyncer can do this for you
-by automatically downloading a list of the configured user's collections::
+The above configuration only syncs a single addressbook.  This is denoted by
+``collections = null`` (collection = addressbook/calendar). We can change this
+line to let vdirsyncer automatically sync all addressbooks it can find::
 
     [pair my_contacts]
     a = my_contacts_local
     b = my_contacts_remote
-    collections = ["from b"]
+    collections = ["from a", "from b"]  # changed from `null`
 
     [storage my_contacts_local]
     type = filesystem
@@ -138,6 +139,8 @@ by automatically downloading a list of the configured user's collections::
 
     [storage my_contacts_remote]
     type = carddav
+
+    # We can simplify this URL here as well. In theory it shouldn't matter.
     url = https://owncloud.example.com/remote.php/carddav/
     username = bob
     password = asdf

tests/cli/test_config.py

@@ -29,6 +29,7 @@ def test_read_config(read_config, monkeypatch):
         b = bob_b
         foo = bar
         bam = true
+        collections = null
 
         [storage bob_a]
         type = filesystem
@@ -45,7 +46,8 @@ def test_read_config(read_config, monkeypatch):
         ''')
 
     assert general == {'status_path': '/tmp/status/'}
-    assert pairs == {'bob': ('bob_a', 'bob_b', {'bam': True, 'foo': 'bar'})}
+    assert pairs == {'bob': ('bob_a', 'bob_b',
+                             {'collections': None, 'bam': True, 'foo': 'bar'})}
     assert storages == {
         'bob_a': {'type': 'filesystem', 'path': '/tmp/contacts/', 'fileext':
                   '.vcf', 'yesno': False, 'number': 42,
@@ -58,6 +60,30 @@ def test_read_config(read_config, monkeypatch):
     assert 'bogus' in errors[0]
 
 
+def test_missing_collections_param(read_config, monkeypatch):
+    errorlog = []
+    monkeypatch.setattr('vdirsyncer.cli.cli_logger.error', errorlog.append)
+
+    with pytest.raises(exceptions.UserError) as excinfo:
+        read_config(u'''
+            [general]
+            status_path = /tmp/status/
+
+            [pair bob]
+            a = bob_a
+            b = bob_b
+
+            [storage bob_a]
+            type = lmao
+
+            [storage bob_b]
+            type = lmao
+            ''')
+
+    assert 'collections parameter missing' in str(excinfo.value)
+    assert not errorlog
+
+
 def test_storage_instance_from_config(monkeypatch):
     def lol(**kw):
         assert kw == {'foo': 'bar', 'baz': 1}
@@ -75,6 +101,7 @@ def test_missing_general_section(read_config):
             [pair my_pair]
             a = my_a
             b = my_b
+            collections = null
 
             [storage my_a]
             type = filesystem

tests/cli/test_main.py

@@ -19,6 +19,7 @@ def test_simple_run(tmpdir, runner):
     [pair my_pair]
     a = my_a
     b = my_b
+    collections = null
 
     [storage my_a]
     type = filesystem
@@ -48,6 +49,7 @@ def test_debug_connections(tmpdir, runner):
     [pair my_pair]
     a = my_a
     b = my_b
+    collections = null
 
     [storage my_a]
     type = filesystem
@@ -75,6 +77,7 @@ def test_empty_storage(tmpdir, runner):
     [pair my_pair]
     a = my_a
     b = my_b
+    collections = null
 
     [storage my_a]
     type = filesystem
@@ -241,6 +244,7 @@ def test_multiple_pairs(tmpdir, runner):
             [pair {a}{b}]
             a = {a}
             b = {b}
+            collections = null
             ''').format(a=name_a, b=name_b)
 
             for name in name_a, name_b:
@@ -326,6 +330,7 @@ def test_ident_conflict(tmpdir, runner):
     [pair foobar]
     a = foo
     b = bar
+    collections = null
 
     [storage foo]
     type = filesystem
@@ -365,6 +370,7 @@ def test_unknown_storage(tmpdir, runner, existing, missing):
     [pair foobar]
     a = foo
     b = bar
+    collections = null
 
     [storage {existing}]
     type = filesystem

vdirsyncer/cli/config.py

@@ -55,15 +55,29 @@ def _validate_general_section(general_config):
 
 
 def _validate_pair_section(pair_config):
-    collections = pair_config.get('collections', None)
+    try:
+        collections = pair_config['collections']
+    except KeyError:
+        raise ValueError('collections parameter missing.\n\n'
+                         'As of 0.9.0 this parameter has no default anymore. '
+                         'Set `collections = null` explicitly in your pair '
+                         'config.')
+
     if collections is None:
         return
+
     e = ValueError('`collections` parameter must be a list of collection '
                    'names (strings!) or `null`.')
-    if not isinstance(collections, list) or \
-       any(not isinstance(x, (text_type, bytes)) for x in collections):
+
+    if not isinstance(collections, list):
         raise e
 
+    if any(not isinstance(x, (text_type, bytes)) for x in collections):
+        raise e
+
+    if len(set(collections)) != len(collections):
+        raise ValueError('Duplicate values in collections parameter.')
+
 
 def load_config():
     fname = os.environ.get('VDIRSYNCER_CONFIG', None)

vdirsyncer/cli/utils.py

@@ -252,14 +252,14 @@ def _handle_collection_not_found(config, collection, e=None):
 
 def _collections_for_pair_impl(status_path, pair):
 
-    shortcuts = set(pair.options.get('collections', ()))
-    if not shortcuts:
+    shortcuts = pair.options['collections']
+    if shortcuts is None:
         yield None, (pair.config_a, pair.config_b)
     else:
         a_discovered = _discover_from_config(pair.config_a)
         b_discovered = _discover_from_config(pair.config_b)
 
-        for shortcut in shortcuts:
+        for shortcut in set(shortcuts):
             if shortcut == 'from a':
                 collections = a_discovered
             elif shortcut == 'from b':