Repository: bids-standard/pybids

Issue:
``build_path`` should check entity values
From the tutorial:
```Python
>>> entities = {
>>>     'subject': '01',
>>>     'run': 2,
>>>     'task': 'n-back',
>>>     'extension': 'tsv.gz',
>>>     'suffix': 'stim'
>>> }
>>> layout.build_path(entities)
'sub-01/func/sub-01_task-n-back_run-2_stim.tsv.gz'
```
(should break for the hyphen in the task value)

Discussion:

Patch:
diff --git a/bids/layout/layout.py b/bids/layout/layout.py
index b00336ea..2d9aede8 100644
--- a/bids/layout/layout.py
+++ b/bids/layout/layout.py
@@ -12,6 +12,7 @@ import sqlite3
 
 import sqlalchemy as sa
 from sqlalchemy.orm import joinedload
+from bids_validator import BIDSValidator
 
 from ..utils import listify, natural_sort, make_bidsfile
 from ..external import inflect, six
@@ -1136,7 +1137,7 @@ class BIDSLayout(object):
         return all_trs.pop()
 
     def build_path(self, source, path_patterns=None, strict=False,
-                   scope='all'):
+                   scope='all', validate=True):
         """Construct a target filename for a file or dictionary of entities.
 
         Args:
@@ -1169,6 +1170,9 @@ class BIDSLayout(object):
                 two or more values are provided, the order determines the
                 precedence of path patterns (i.e., earlier layouts will have
                 higher precedence).
+            validate (bool): If True, built path must pass BIDS validator. If
+                False, no validation is attempted, and an invalid path may be
+                returned (e.g., if an entity value contains a hyphen).
         """
         # 'is_file' is a crude check for Path objects
         if isinstance(source, six.string_types) or hasattr(source, 'is_file'):
@@ -1193,7 +1197,15 @@ class BIDSLayout(object):
                         path_patterns.extend(c.default_path_patterns)
                     seen_configs.add(c)
 
-        return build_path(source, path_patterns, strict)
+        built = build_path(source, path_patterns, strict)
+        to_check = os.path.join(os.path.sep, built)
+
+        if not validate or BIDSValidator().is_bids(to_check):
+            return built
+
+        raise ValueError("Built path {} is not a valid BIDS filename. Please "
+                         "make sure all provided entity values are "
+                         "spec-compliant.".format(built))
 
     def copy_files(self, files=None, path_patterns=None, symbolic_links=True,
                    root=None, conflicts='fail', **kwargs):
@@ -1232,7 +1244,7 @@ class BIDSLayout(object):
     def write_contents_to_file(self, entities, path_patterns=None,
                                contents=None, link_to=None,
                                content_mode='text', conflicts='fail',
-                               strict=False):
+                               strict=False, validate=True):
         """Write arbitrary data to a file defined by the passed entities and
         path patterns.
 
@@ -1256,8 +1268,12 @@ class BIDSLayout(object):
                     'append': adds  a suffix to each file copy, starting with 1
             strict (bool): If True, all entities must be matched inside a
                 pattern in order to be a valid match. If False, extra entities
+            validate (bool): If True, built path must pass BIDS validator. If
+                False, no validation is attempted, and an invalid path may be
+                returned (e.g., if an entity value contains a hyphen).
         """
-        path = self.build_path(entities, path_patterns, strict)
+        path = self.build_path(entities, path_patterns, strict,
+                               validate=validate)
 
         if path is None:
             raise ValueError("Cannot construct any valid filename for "
diff --git a/examples/pybids_tutorial.ipynb b/examples/pybids_tutorial.ipynb
index 3f810558..aa17252a 100644
--- a/examples/pybids_tutorial.ipynb
+++ b/examples/pybids_tutorial.ipynb
@@ -93,14 +93,14 @@
     {
      "data": {
       "text/plain": [
-       "[<BIDSFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/dataset_description.json'>,\n",
+       "[<BIDSJSONFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/dataset_description.json'>,\n",
        " <BIDSDataFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/participants.tsv'>,\n",
        " <BIDSFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/README'>,\n",
        " <BIDSImageFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/sub-01/ses-1/anat/sub-01_ses-1_T1map.nii.gz'>,\n",
        " <BIDSImageFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/sub-01/ses-1/anat/sub-01_ses-1_T1w.nii.gz'>,\n",
        " <BIDSImageFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/sub-01/ses-1/fmap/sub-01_ses-1_run-1_magnitude1.nii.gz'>,\n",
        " <BIDSImageFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/sub-01/ses-1/fmap/sub-01_ses-1_run-1_magnitude2.nii.gz'>,\n",
-       " <BIDSFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/sub-01/ses-1/fmap/sub-01_ses-1_run-1_phasediff.json'>,\n",
+       " <BIDSJSONFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/sub-01/ses-1/fmap/sub-01_ses-1_run-1_phasediff.json'>,\n",
        " <BIDSImageFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/sub-01/ses-1/fmap/sub-01_ses-1_run-1_phasediff.nii.gz'>,\n",
        " <BIDSImageFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/sub-01/ses-1/fmap/sub-01_ses-1_run-2_magnitude1.nii.gz'>]"
       ]
@@ -480,7 +480,7 @@
     {
      "data": {
       "text/plain": [
-       "[<BIDSFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/task-rest_acq-fullbrain_run-1_physio.json'>,\n",
+       "[<BIDSJSONFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/task-rest_acq-fullbrain_run-1_physio.json'>,\n",
        " <BIDSImageFile filename='/Users/tal/Dropbox/Code/pybids/bids/tests/data/7t_trt/sub-01/ses-1/func/sub-01_ses-1_task-rest_acq-fullbrain_run-1_bold.nii.gz'>]"
       ]
      },
@@ -781,7 +781,7 @@
     {
      "data": {
       "text/plain": [
-       "'sub-01_task-n-back_run-2_z.nii.gz'"
+       "'sub-01_task-nback_run-2_z.nii.gz'"
       ]
      },
      "execution_count": 18,
@@ -796,12 +796,19 @@
     "entities = {\n",
     "    'subject': '01',\n",
     "    'run': 2,\n",
-    "    'task': 'n-back',\n",
+    "    'task': 'nback',\n",
     "    'suffix': 'z'\n",
     "}\n",
     "\n",
     "# Notice we pass the new pattern as the second argument\n",
-    "layout.build_path(entities, pattern)\n"
+    "layout.build_path(entities, pattern, validate=False)\n"
+   ]
+  },
+  {
+   "cell_type": "markdown",
+   "metadata": {},
+   "source": [
+    "Note that in the above example, we set `validate=False` to ensure that the standard BIDS file validator doesn't run (because the pattern we defined isn't actually compliant with the BIDS specification)."
    ]
   },
   {
@@ -1576,8 +1583,8 @@
        "    <tr style=\"text-align: right;\">\n",
        "      <th></th>\n",
        "      <th>suffix</th>\n",
-       "      <th>subject</th>\n",
        "      <th>session</th>\n",
+       "      <th>subject</th>\n",
        "      <th>CCPT_FN_count</th>\n",
        "      <th>CCPT_FP_count</th>\n",
        "      <th>CCPT_avg_FN_RT</th>\n",
@@ -1602,8 +1609,8 @@
        "    <tr>\n",
        "      <th>0</th>\n",
        "      <td>sessions</td>\n",
-       "      <td>01</td>\n",
        "      <td>1</td>\n",
+       "      <td>01</td>\n",
        "      <td>0.0</td>\n",
        "      <td>1.0</td>\n",
        "      <td>NaN</td>\n",
@@ -1626,32 +1633,8 @@
        "    <tr>\n",
        "      <th>1</th>\n",
        "      <td>sessions</td>\n",
-       "      <td>01</td>\n",
-       "      <td>2</td>\n",
-       "      <td>0.0</td>\n",
-       "      <td>1.0</td>\n",
-       "      <td>NaN</td>\n",
-       "      <td>595.0</td>\n",
-       "      <td>503.333333</td>\n",
-       "      <td>96.0</td>\n",
-       "      <td>2.0</td>\n",
-       "      <td>...</td>\n",
-       "      <td>5.0</td>\n",
-       "      <td>100.0</td>\n",
-       "      <td>1.0</td>\n",
-       "      <td>20.0</td>\n",
-       "      <td>101.0</td>\n",
-       "      <td>101.0</td>\n",
-       "      <td>9.0</td>\n",
-       "      <td>7.0</td>\n",
-       "      <td>100.0</td>\n",
-       "      <td>100.0</td>\n",
-       "    </tr>\n",
-       "    <tr>\n",
-       "      <th>2</th>\n",
-       "      <td>sessions</td>\n",
-       "      <td>02</td>\n",
        "      <td>1</td>\n",
+       "      <td>02</td>\n",
        "      <td>0.0</td>\n",
        "      <td>5.0</td>\n",
        "      <td>NaN</td>\n",
@@ -1672,34 +1655,10 @@
        "      <td>100.0</td>\n",
        "    </tr>\n",
        "    <tr>\n",
-       "      <th>3</th>\n",
-       "      <td>sessions</td>\n",
-       "      <td>02</td>\n",
-       "      <td>2</td>\n",
-       "      <td>1.0</td>\n",
-       "      <td>0.0</td>\n",
-       "      <td>0.0</td>\n",
-       "      <td>NaN</td>\n",
-       "      <td>366.315789</td>\n",
-       "      <td>95.0</td>\n",
-       "      <td>0.0</td>\n",
-       "      <td>...</td>\n",
-       "      <td>5.0</td>\n",
-       "      <td>60.0</td>\n",
-       "      <td>2.0</td>\n",
-       "      <td>60.0</td>\n",
-       "      <td>108.0</td>\n",
-       "      <td>106.0</td>\n",
-       "      <td>4.0</td>\n",
-       "      <td>3.0</td>\n",
-       "      <td>100.0</td>\n",
-       "      <td>80.0</td>\n",
-       "    </tr>\n",
-       "    <tr>\n",
-       "      <th>4</th>\n",
+       "      <th>2</th>\n",
        "      <td>sessions</td>\n",
-       "      <td>03</td>\n",
        "      <td>1</td>\n",
+       "      <td>03</td>\n",
        "      <td>0.0</td>\n",
        "      <td>1.0</td>\n",
        "      <td>NaN</td>\n",
@@ -1719,46 +1678,94 @@
        "      <td>100.0</td>\n",
        "      <td>0.0</td>\n",
        "    </tr>\n",
+       "    <tr>\n",
+       "      <th>3</th>\n",
+       "      <td>sessions</td>\n",
+       "      <td>1</td>\n",
+       "      <td>04</td>\n",
+       "      <td>0.0</td>\n",
+       "      <td>1.0</td>\n",
+       "      <td>NaN</td>\n",
+       "      <td>443.0</td>\n",
+       "      <td>417.906250</td>\n",
+       "      <td>96.0</td>\n",
+       "      <td>0.1</td>\n",
+       "      <td>...</td>\n",
+       "      <td>5.0</td>\n",
+       "      <td>80.0</td>\n",
+       "      <td>4.0</td>\n",
+       "      <td>0.0</td>\n",
+       "      <td>130.0</td>\n",
+       "      <td>110.0</td>\n",
+       "      <td>6.0</td>\n",
+       "      <td>5.0</td>\n",
+       "      <td>100.0</td>\n",
+       "      <td>85.0</td>\n",
+       "    </tr>\n",
+       "    <tr>\n",
+       "      <th>4</th>\n",
+       "      <td>sessions</td>\n",
+       "      <td>1</td>\n",
+       "      <td>05</td>\n",
+       "      <td>0.0</td>\n",
+       "      <td>2.0</td>\n",
+       "      <td>NaN</td>\n",
+       "      <td>355.5</td>\n",
+       "      <td>372.114583</td>\n",
+       "      <td>96.0</td>\n",
+       "      <td>0.0</td>\n",
+       "      <td>...</td>\n",
+       "      <td>5.0</td>\n",
+       "      <td>75.0</td>\n",
+       "      <td>5.0</td>\n",
+       "      <td>80.0</td>\n",
+       "      <td>105.0</td>\n",
+       "      <td>117.0</td>\n",
+       "      <td>7.0</td>\n",
+       "      <td>7.0</td>\n",
+       "      <td>60.0</td>\n",
+       "      <td>30.0</td>\n",
+       "    </tr>\n",
        "  </tbody>\n",
        "</table>\n",
        "<p>5 rows × 97 columns</p>\n",
        "</div>"
       ],
       "text/plain": [
-       "     suffix subject session  CCPT_FN_count  CCPT_FP_count  CCPT_avg_FN_RT  \\\n",
-       "0  sessions      01       1            0.0            1.0             NaN   \n",
-       "1  sessions      01       2            0.0            1.0             NaN   \n",
-       "2  sessions      02       1            0.0            5.0             NaN   \n",
-       "3  sessions      02       2            1.0            0.0             0.0   \n",
-       "4  sessions      03       1            0.0            1.0             NaN   \n",
+       "     suffix session subject  CCPT_FN_count  CCPT_FP_count  CCPT_avg_FN_RT  \\\n",
+       "0  sessions       1      01            0.0            1.0             NaN   \n",
+       "1  sessions       1      02            0.0            5.0             NaN   \n",
+       "2  sessions       1      03            0.0            1.0             NaN   \n",
+       "3  sessions       1      04            0.0            1.0             NaN   \n",
+       "4  sessions       1      05            0.0            2.0             NaN   \n",
        "\n",
        "   CCPT_avg_FP_RT  CCPT_avg_succ_RT  CCPT_succ_count  caffeine_daily  ...  \\\n",
        "0           507.0        500.770833             96.0             0.5  ...   \n",
-       "1           595.0        503.333333             96.0             2.0  ...   \n",
-       "2           297.6        351.729167             96.0             0.0  ...   \n",
-       "3             NaN        366.315789             95.0             0.0  ...   \n",
-       "4           441.0        426.718750             96.0             1.0  ...   \n",
+       "1           297.6        351.729167             96.0             0.0  ...   \n",
+       "2           441.0        426.718750             96.0             1.0  ...   \n",
+       "3           443.0        417.906250             96.0             0.1  ...   \n",
+       "4           355.5        372.114583             96.0             0.0  ...   \n",
        "\n",
        "   relative_water_intake  specific_vague  subject_id  surroundings  \\\n",
        "0                    7.0            95.0         1.0           0.0   \n",
-       "1                    5.0           100.0         1.0          20.0   \n",
-       "2                    1.0           100.0         2.0          70.0   \n",
-       "3                    5.0            60.0         2.0          60.0   \n",
-       "4                    3.0           100.0         3.0          10.0   \n",
+       "1                    1.0           100.0         2.0          70.0   \n",
+       "2                    3.0           100.0         3.0          10.0   \n",
+       "3                    5.0            80.0         4.0           0.0   \n",
+       "4                    5.0            75.0         5.0          80.0   \n",
        "\n",
        "   systolic_blood_pressure_left  systolic_blood_pressure_right  thirst  \\\n",
        "0                         108.0                          109.0     9.0   \n",
-       "1                         101.0                          101.0     9.0   \n",
-       "2                          99.0                          100.0     2.0   \n",
-       "3                         108.0                          106.0     4.0   \n",
-       "4                         122.0                          128.0     3.0   \n",
+       "1                          99.0                          100.0     2.0   \n",
+       "2                         122.0                          128.0     3.0   \n",
+       "3                         130.0                          110.0     6.0   \n",
+       "4                         105.0                          117.0     7.0   \n",
        "\n",
        "   vigilance  vigilance_nyc-q  words  \n",
        "0        9.0            100.0  100.0  \n",
        "1        7.0            100.0  100.0  \n",
-       "2        7.0            100.0  100.0  \n",
-       "3        3.0            100.0   80.0  \n",
-       "4        8.0            100.0    0.0  \n",
+       "2        8.0            100.0    0.0  \n",
+       "3        5.0            100.0   85.0  \n",
+       "4        7.0             60.0   30.0  \n",
        "\n",
        "[5 rows x 97 columns]"
       ]


Test patch:
diff --git a/bids/layout/tests/test_path_building.py b/bids/layout/tests/test_path_building.py
index 2e53be75..e498f60e 100644
--- a/bids/layout/tests/test_path_building.py
+++ b/bids/layout/tests/test_path_building.py
@@ -14,4 +14,14 @@ def test_bold_construction(layout):
     ents = dict(subject='01', run=1, task='rest', suffix='bold')
     assert layout.build_path(ents) == "sub-01/func/sub-01_task-rest_run-1_bold.nii.gz"
     ents['acquisition'] = 'random'
-    assert layout.build_path(ents) == "sub-01/func/sub-01_task-rest_acq-random_run-1_bold.nii.gz"
\ No newline at end of file
+    assert layout.build_path(ents) == "sub-01/func/sub-01_task-rest_acq-random_run-1_bold.nii.gz"
+
+
+def test_invalid_file_construction(layout):
+    # no hyphens allowed!
+    ents = dict(subject='01', run=1, task='resting-state', suffix='bold')
+    with pytest.raises(ValueError):
+        layout.build_path(ents)
+    
+    target = "sub-01/func/sub-01_task-resting-state_run-1_bold.nii.gz"
+    assert layout.build_path(ents, validate=False) == target
diff --git a/bids/layout/tests/test_writing.py b/bids/layout/tests/test_writing.py
index 939b5c3b..6883afbc 100644
--- a/bids/layout/tests/test_writing.py
+++ b/bids/layout/tests/test_writing.py
@@ -108,14 +108,14 @@ class TestWritableFile:
         assert build_path(writable_file.entities, pats) == target
 
         # Pattern with default value
-        pats = ['sess-{session|A}/r-{run}.nii.gz']
-        assert build_path({'run': 3}, pats) == 'sess-A/r-3.nii.gz'
+        pats = ['ses-{session|A}/r-{run}.nii.gz']
+        assert build_path({'run': 3}, pats) == 'ses-A/r-3.nii.gz'
 
         # Pattern with both valid and default values
-        pats = ['sess-{session<A|B|C>|D}/r-{run}.nii.gz']
-        assert build_path({'session': 1, 'run': 3}, pats) == 'sess-D/r-3.nii.gz'
-        pats = ['sess-{session<A|B|C>|D}/r-{run}.nii.gz']
-        assert build_path({'session': 'B', 'run': 3}, pats) == 'sess-B/r-3.nii.gz'
+        pats = ['ses-{session<A|B|C>|D}/r-{run}.nii.gz']
+        assert build_path({'session': 1, 'run': 3}, pats) == 'ses-D/r-3.nii.gz'
+        pats = ['ses-{session<A|B|C>|D}/r-{run}.nii.gz']
+        assert build_path({'session': 'B', 'run': 3}, pats) == 'ses-B/r-3.nii.gz'
 
     def test_strict_build_path(self):
 
@@ -181,20 +181,20 @@ class TestWritableLayout:
 
         tmpdir = str(tmp_bids)
         pat = join(str(tmpdir), 'sub-{subject<02>}'
-                                '/sess-{session}'
+                                '/ses-{session}'
                                 '/r-{run}'
                                 '/suffix-{suffix}'
                                 '/acq-{acquisition}'
                                 '/task-{task}.nii.gz')
         layout.copy_files(path_patterns=pat)
         example_file = join(str(tmpdir), 'sub-02'
-                                         '/sess-2'
+                                         '/ses-2'
                                          '/r-1'
                                          '/suffix-bold'
                                          '/acq-fullbrain'
                                          '/task-rest.nii.gz')
         example_file2 = join(str(tmpdir), 'sub-01'
-                                          '/sess-2'
+                                          '/ses-2'
                                           '/r-1'
                                           '/suffix-bold'
                                           '/acq-fullbrain'
@@ -204,12 +204,12 @@ class TestWritableLayout:
         assert not exists(example_file2)
 
         pat = join(str(tmpdir), 'sub-{subject<01>}'
-                                '/sess-{session}'
+                                '/ses-{session}'
                                 '/r-{run}'
                                 '/suffix-{suffix}'
                                 '/task-{task}.nii.gz')
         example_file = join(str(tmpdir), 'sub-01'
-                                         '/sess-2'
+                                         '/ses-2'
                                          '/r-1'
                                          '/suffix-bold'
                                          '/task-rest.nii.gz')
@@ -227,10 +227,10 @@ class TestWritableLayout:
     def test_write_contents_to_file(self, tmp_bids, layout):
         contents = 'test'
         entities = {'subject': 'Bob', 'session': '01'}
-        pat = join('sub-{subject}/sess-{session}/desc.txt')
+        pat = join('sub-{subject}/ses-{session}/desc.txt')
         layout.write_contents_to_file(entities, path_patterns=pat,
-                                      contents=contents)
-        target = join(str(tmp_bids), 'bids', 'sub-Bob/sess-01/desc.txt')
+                                      contents=contents, validate=False)
+        target = join(str(tmp_bids), 'bids', 'sub-Bob/ses-01/desc.txt')
         assert exists(target)
         with open(target) as f:
             written = f.read()
@@ -253,13 +253,13 @@ class TestWritableLayout:
     def test_build_file_from_layout(self, tmpdir, layout):
         entities = {'subject': 'Bob', 'session': '01', 'run': '1'}
         pat = join(str(tmpdir), 'sub-{subject}'
-                   '/sess-{session}'
+                   '/ses-{session}'
                    '/r-{run}.nii.gz')
-        path = layout.build_path(entities, path_patterns=pat)
-        assert path == join(str(tmpdir), 'sub-Bob/sess-01/r-1.nii.gz')
+        path = layout.build_path(entities, path_patterns=pat, validate=False)
+        assert path == join(str(tmpdir), 'sub-Bob/ses-01/r-1.nii.gz')
 
         data_dir = join(dirname(__file__), 'data', '7t_trt')
         filename = 'sub-04_ses-1_task-rest_acq-fullbrain_run-1_physio.tsv.gz'
         file = join('sub-04', 'ses-1', 'func', filename)
-        path = layout.build_path(file, path_patterns=pat)
-        assert path.endswith('sub-04/sess-1/r-1.nii.gz')
+        path = layout.build_path(file, path_patterns=pat, validate=False)
+        assert path.endswith('sub-04/ses-1/r-1.nii.gz')