Repository: lexibank/pylexibank

Issue:
formspec replacements
I'm liking the formspec way of handling form cleaning, but notice that a lot of datasets do manual stripping/replacing of characters e.g. robinsonap has a proto-language where all forms have a leading `*` to indicate they're reconstructions.  It's easy to do this with string replace, but its not clear when to do this (before or after formspec?).

Is it worth adding an extra argument to formspec of `replacements` e.g. 

```python
    form_spec = FormSpec(
        brackets={"[": "]", "{": "}", "(": ")"},
        separators=";/,",
        missing_data=('?', '-', '', ''),
        replacements = {
            "*": "",   # remove *
            "ts": "ʦ",   # replace characters with digraph
        },
        strip_inside_brackets=True
    )
````

The default would be `{}`, but this could be easily overridden in datasets and means our replacement code would then be consistent too.


Discussion:
xrotwang: I'm inclined to add multi-space normalization (#146) unconditionally - what do you think?
chrzyki: Awesome! Thanks! The 'trap' warning is absolutely justified, e.g. I've run into this myself already in https://github.com/lexibank/ids/blob/master/lexibank_ids.py. As soon as this is merged, I'll update my datasets to use this replacement variant.
chrzyki: > I'm inclined to add multi-space normalization (#146) unconditionally - what do you think?

Yep! I think that would be good!
LinguList: Yes, this looks good, but what do we do with FirstFormOnlySpec? Should we include that with a true/false flag into the FormSpec, as this would also allow us to loose code, and make it traceable what one does, or do we leave it as such?
xrotwang: Ok, I'll add a first-form-only attribute, and flags to do white-space normalization and unicode normalization. As long as this stays below - say - 10 attributes, the class should still be transparent enough.
LinguList: For the methodology part of a lexibank paper, it will be useful to represent our changes in form of a decision tree, as this is basically what we do here, right? The advantage of flags is that, if we can go on with this, we can see and handle as much data as possible in a uniform manner, which is a great advantage of custom code or customized FormSpec classes.
SimonGreenhill: Yes, it could even be useful to log the Formspec to README.md? 
LinguList: Yes, @SimonGreenhill, I think that would be helpful, and reflecting what I tried by then in our google doc, where I wrote down what defaults had been used for splitting. Now with FormSpec, we have this all transparently in one place. 

A table that lists attribute / parameter would probably be all we need.
SimonGreenhill: Just came across this in `robinsonap`:

```csv
blagar-182_mango-1,,blagar,182_mango,"biyát ‘wild mango’, mampleʔ ‘domestic mango’",biyát,b i j a t,,Robinson2012,,
blagar-182_mango-2,,blagar,182_mango,"biyát ‘wild mango’, mampleʔ ‘domestic mango’",mampleʔ,m a m p l e ʔ,,Robinson2012,,
```

the second form, split from `biyát ‘wild mango’, mampleʔ ‘domestic mango’`, becomes ` mample?` (i.e. leading space). Can we also lstrip()?
codecov-io: # [Codecov](https://codecov.io/gh/lexibank/pylexibank/pull/148?src=pr&el=h1) Report
> Merging [#148](https://codecov.io/gh/lexibank/pylexibank/pull/148?src=pr&el=desc) into [master](https://codecov.io/gh/lexibank/pylexibank/commit/9d3001127a62fc063845f02a5afafd43a5be7234?src=pr&el=desc) will **increase** coverage by `0.05%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/lexibank/pylexibank/pull/148/graphs/tree.svg?width=650&token=v0oYsvytpU&height=150&src=pr)](https://codecov.io/gh/lexibank/pylexibank/pull/148?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #148      +/-   ##
==========================================
+ Coverage   97.31%   97.36%   +0.05%     
==========================================
  Files          26       26              
  Lines        1415     1444      +29     
==========================================
+ Hits         1377     1406      +29     
  Misses         38       38
```


| [Impacted Files](https://codecov.io/gh/lexibank/pylexibank/pull/148?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/pylexibank/forms.py](https://codecov.io/gh/lexibank/pylexibank/pull/148/diff?src=pr&el=tree#diff-c3JjL3B5bGV4aWJhbmsvZm9ybXMucHk=) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/lexibank/pylexibank/pull/148?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/lexibank/pylexibank/pull/148?src=pr&el=footer). Last update [9d30011...8c34fc7](https://codecov.io/gh/lexibank/pylexibank/pull/148?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

xrotwang: The output of `FormSpec.as_markdown` looks like this:

## Specification of form manipulation


Specification of the value-to-form processing in Lexibank datasets:

The value-to-form processing is divided into two steps, implemented as methods:
- `FormSpec.split`: Splits a string into individual form chunks.
- `FormSpec.clean`: Normalizes a form chunk.

These methods use the attributes of a `FormSpec` instance to configure their behaviour.

- `brackets`: `{'(': ')'}`
  Pairs of strings that should be recognized as brackets, specified as `dict` mapping opening string to closing string
- `separators`: `;/,`
  Iterable of single character tokens that should be recognized as word separator
- `missing_data`: `('?', '-')`
  Iterable of strings that are used to mark missing data
- `strip_inside_brackets`: `True`
  Flag signaling whether to strip content in brackets (**and** strip leading and trailing whitespace)
- `replacements`: `[]`
  List of pairs (`source`, `target`) used to replace occurrences of `source` in formswith `target` (before stripping content in brackets)
- `first_form_only`: `False`
  Flag signaling whether at most one form should be returned from `split` - effectively ignoring any spelling variants, etc.
- `normalize_whitespace`: `True`
  Flag signaling whether to normalize whitespace - stripping leading and trailing whitespace and collapsing multi-character whitespace to single spaces
- `normalize_unicode`: `None`
  UNICODE normalization form to use for input of `split` (`None`, 'NFD' or 'NFC')

xrotwang: I'd propose to **not** add this to `README.md`, but rather to a saparate `FORMS.md`?
SimonGreenhill: Great idea, thanks @xrotwang. Minor comment, can we change the two iterables  (separators and missing_data) to be the same format by default just for consistency? 

```
separators: ;/,
Iterable of single character tokens that should be recognized as word separator
missing_data: ('?', '-')
```

e.g. separators = (';', '/', ','),
LinguList: @SimonGreenhill, the nature is different though: separators can only be single characters, at least according to the current regex, or do we switch to potential multi-character separators?
SimonGreenhill: ah, ok, if there's an implementation difference, then that's fine. 

(I guess multichar separators are not impossible though: "and", "or")
xrotwang: @SimonGreenhill for `clldutils.split_text_with_content` they are impossible, unfortunately. Do you think this is grave/urgent enough a problem to be addressed right now?
xrotwang: @SimonGreenhill For now, I think, sticking with one-character separators is a lot better defined. For `and`, `or`, etc. there's always the question of requiring enclosing whitespace or not (and possibly other edge cases).
SimonGreenhill: No, not urgently (or ever). 
LinguList: I agree with @xrotwang, I am just checking bowernpny, and I found many replacements which are used but occur only, say 10 times in the data. So my rule would be: if we encounter something along those dimensions, we should put more explicit changes into lexemes.tsv to avoid having seemingly targeted code for few exceptions, and this is what we may often encounter (also when people use multi-character separators).
tresoldi: Yes, agreed. And bowernpny was one of the cases of multiple replacements I had in mind, for the obvious reason that I wrote those.

Replacements in FormSpec should be used but not abused, as already discussed.
xrotwang: Ok, I'm merging this now.

Patch:
diff --git a/src/pylexibank/dataset.py b/src/pylexibank/dataset.py
index 07113dc..bac565d 100644
--- a/src/pylexibank/dataset.py
+++ b/src/pylexibank/dataset.py
@@ -218,6 +218,7 @@ class Dataset(BaseDataset):
         tr = self.cldf_dir / '.transcription-report.json'
         tr = jsonlib.load(tr) if tr.exists() else None
         res += report.report(self, tr, getattr(args, 'glottolog', None), args.log)
+        self.dir.write('FORMS.md', self.form_spec.as_markdown())
         return res
 
 
diff --git a/src/pylexibank/forms.py b/src/pylexibank/forms.py
index 3553d90..d365602 100644
--- a/src/pylexibank/forms.py
+++ b/src/pylexibank/forms.py
@@ -1,3 +1,5 @@
+import re
+import unicodedata
 import logging
 
 import attr
@@ -8,18 +10,98 @@ __all__ = ['FormSpec', 'FirstFormOnlySpec']
 log = logging.getLogger('pylexibank')
 
 
+def valid_replacements(instance, attribute, value):
+    if not isinstance(value, list):
+        raise ValueError('replacements must be list of pairs')
+    for v in value:
+        if not (isinstance(v, tuple)
+                and len(v) == 2
+                and isinstance(v[0], str)
+                and isinstance(v[1], str)):
+            raise ValueError('replacements must be list of pairs')
+
+
+def valid_separators(instance, attribute, value):
+    if not isinstance(value, str):
+        if not isinstance(value, (list, tuple)):
+            raise ValueError('separators must be an iterable of single character strings')
+        for v in value:
+            if (not isinstance(v, str)) or len(v) > 1:
+                raise ValueError('separators must be an iterable of single character strings')
+
+
+def attrib(help, **kw):
+    kw['metadata'] = dict(help=help)
+    return attr.ib(**kw)
+
+
 @attr.s
 class FormSpec(object):
-    brackets = attr.ib(
+    """
+    Specification of the value-to-form processing in Lexibank datasets:
+
+    The value-to-form processing is divided into two steps, implemented as methods:
+    - `FormSpec.split`: Splits a string into individual form chunks.
+    - `FormSpec.clean`: Normalizes a form chunk.
+
+    These methods use the attributes of a `FormSpec` instance to configure their behaviour.
+    """
+    brackets = attrib(
+        "Pairs of strings that should be recognized as brackets, specified as `dict` "
+        "mapping opening string to closing string",
         default={"(": ")"},
-        validator=attr.validators.instance_of(dict))
-    separators = attr.ib(default=";/,")
-    missing_data = attr.ib(
+        validator=attr.validators.instance_of(dict),
+    )
+    separators = attrib(
+        "Iterable of single character tokens that should be recognized as word separator",
+        default=(";", "/", ","),
+        validator=valid_separators,
+    )
+    missing_data = attrib(
+        "Iterable of strings that are used to mark missing data",
         default=('?', '-'),
-        validator=attr.validators.instance_of((tuple, list)))
-    strip_inside_brackets = attr.ib(
+        validator=attr.validators.instance_of((tuple, list)),
+    )
+    strip_inside_brackets = attrib(
+        "Flag signaling whether to strip content in brackets "
+        "(**and** strip leading and trailing whitespace)",
         default=True,
         validator=attr.validators.instance_of(bool))
+    replacements = attrib(
+        "List of pairs (`source`, `target`) used to replace occurrences of `source` in forms"
+        "with `target` (before stripping content in brackets)",
+        default=attr.Factory(list),
+        validator=valid_replacements)
+    first_form_only = attrib(
+        "Flag signaling whether at most one form should be returned from `split` - "
+        "effectively ignoring any spelling variants, etc.",
+        default=False,
+        validator=attr.validators.instance_of(bool),
+    )
+    normalize_whitespace = attrib(
+        "Flag signaling whether to normalize whitespace - stripping leading and trailing "
+        "whitespace and collapsing multi-character whitespace to single spaces",
+        default=True,
+        validator=attr.validators.instance_of(bool),
+    )
+    normalize_unicode = attrib(
+        "UNICODE normalization form to use for input of `split` (`None`, 'NFD' or 'NFC')",
+        default=None,
+        validator=attr.validators.in_(['NFD', 'NFC', None]),
+    )
+
+    def as_markdown(self):
+        """
+        :return: Description of `FormSpec` in markdown.
+        """
+        res = ['## Specification of form manipulation\n']
+        res.extend([line.strip() for line in self.__class__.__doc__.splitlines()])
+        for field in attr.fields(self.__class__):
+            res.extend([
+                '- `{0}`: `{1}`'.format(field.name, getattr(self, field.name)),
+                '  {0}'.format(field.metadata['help'])
+            ])
+        return '\n'.join(res)
 
     def clean(self, form, item=None):
         """
@@ -29,6 +111,10 @@ class FormSpec(object):
         :return: None to skip the form, or the cleaned form as string.
         """
         if form not in self.missing_data:
+            if self.normalize_whitespace:
+                form = re.sub(r'\s+', ' ', form.strip())
+            for source, target in self.replacements:
+                form = form.replace(source, target)
             if self.strip_inside_brackets:
                 return text.strip_brackets(form, brackets=self.brackets)
             return form
@@ -38,13 +124,18 @@ class FormSpec(object):
         if value in lexemes:
             log.debug('overriding via lexemes.csv: %r -> %r' % (value, lexemes[value]))
             value = lexemes[value]
-        return misc.nfilter(
+        if self.normalize_unicode:
+            value = unicodedata.normalize(self.normalize_unicode, value)
+        res = misc.nfilter(
             self.clean(form, item=item)
             for form in text.split_text_with_context(
                 value, separators=self.separators, brackets=self.brackets))
+        if self.first_form_only:
+            return res[:1]
+        return res
 
 
 @attr.s
 class FirstFormOnlySpec(FormSpec):
-    def split(self, *args, **kw):
-        return super().split(*args, **kw)[:1]
+    def __attrs_post_init__(self):
+        self.first_form_only = True


Test patch:
diff --git a/tests/test_commands.py b/tests/test_commands.py
index ee50914..93d6008 100644
--- a/tests/test_commands.py
+++ b/tests/test_commands.py
@@ -60,6 +60,7 @@ def test_init_profile(dataset, repos):
 
 def test_readme(dataset, repos):
     _main('lexibank.readme {0} --glottolog {1}'.format(str(dataset.dir / 'td.py'), repos))
+    assert dataset.dir.joinpath('FORMS.md').exists()
 
 
 def test_new(tmpdir, mocker):
diff --git a/tests/test_forms.py b/tests/test_forms.py
index e9093c0..67a9f45 100644
--- a/tests/test_forms.py
+++ b/tests/test_forms.py
@@ -1,3 +1,5 @@
+import pytest
+
 from pylexibank.forms import *
 
 
@@ -9,5 +11,40 @@ def test_split():
     spec.strip_inside_brackets = False
     assert spec.split({}, 'x(a)') == ['x(a)']
 
+    spec = FormSpec(separators='|')
+    assert spec.split(None, 'x|y') == ['x', 'y']
+    assert spec.split(None, 'x;y') == ['x;y']
+
     spec = FirstFormOnlySpec()
     assert spec.split({}, 'x', lexemes={'x': 'x;y'}) == ['x']
+
+
+def test_markdown():
+    spec = FormSpec()
+    assert spec.as_markdown()
+
+
+def test_normalize_whitespace():
+    spec = FormSpec()
+    assert spec.clean(' a\t b\n') == 'a b'
+
+    spec = FormSpec(normalize_whitespace=False, strip_inside_brackets=False)
+    assert spec.clean(' a\t b\n') == ' a\t b\n'
+
+
+def test_normalize_unicode():
+    spec = FormSpec(normalize_unicode='NFD', separators='\u0308')
+    # The combining diaresis is used as separator:
+    assert len(spec.split(None, 'äb')) == 2
+
+
+def test_replacements():
+    spec = FormSpec(replacements=[('x', 'y')])
+    assert spec.clean('x') == 'y'
+    assert spec.clean('y') == 'y'
+
+    with pytest.raises(ValueError):
+        FormSpec(replacements=())
+
+    with pytest.raises(ValueError):
+        FormSpec(replacements=[(1, 2)])