Repository: goodmami/wn

Issue:
Validate for empty or ill-formatted definitions and examples
The MCR wordnet candidate had some interesting issues with definitions, although they probably apply more broadly (definitely to examples).   I don't think these are bugs, but possibly something we should add a warning for?   I think there are are two issues, neither of which are illegal XML.

1. Definition contains only whitespace or is empty:
```
		<Synset id="spa-30-80001224-n" ili="ili-30-80001224-n">
			<Definition>
				
			</Definition>
		</Synset>
```

Here we should warn something like 'Definition for synset {ID} is empty, better to omit'.

2. Definition has whitespace before and after:
```
<Synset id="spa-30-80001223-n" ili="ili-30-80001223-n">
			<Definition>
				Pequeña malformación que causa la dilatación y fragilidad vascular del colon, dando como resultado una pérdida intermitente de sangre desde el tracto intestinal.
			</Definition>
		</Synset>
```
Maybe here we should warn something like 'Definition for synset {ID} contains unnecessary whitespace'.

@jmccrae should we add documentation to the https://github.com/globalwordnet/schemas saying the best practice is not to pad, and to omit empty definitions (and examples), or is this too obvious?

@goodmami should we strip the text of padding before adding it to the database?

Hints from issue comments:
@goodmami: Are you talking about warning during validation or during normal use of Wn? To me, the former seems acceptable but not the latter, as this is just bad formatting and not something that makes the data less correct or usable.

In Wn, I try to store in the database an accurate representation of what was in the WN-LMF file, such that exporting the data would result in an equivalent WN-LMF file, so I don't think stripping the definitions is a good solution. However, it would be fine with me if OMW wanted to fix these things during the compilation of its wordnets.@fcbond: Hi,

I was indeed thinking of warning during validation.

I think returning \n\t\t\n\t\t\n for the definition of a synset, rather
than None, is less correct and does make it less usable.  However, as you
say, the ideal time to catch this is when the wordnet is made, not when we
load.


On Sun, Nov 7, 2021 at 12:27 PM Michael Wayne Goodman <
***@***.***> wrote:

> Are you talking about warning during validation or during normal use of
> Wn? To me, the former seems acceptable but not the latter, as this is just
> bad formatting and not something that makes the data less correct or usable.
>
> In Wn, I try to store in the database an accurate representation of what
> was in the WN-LMF file, such that exporting the data would result in an
> equivalent WN-LMF file, so I don't think stripping the definitions is a
> good solution. However, it would be fine with me if OMW wanted to fix these
> things during the compilation of its wordnets.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/goodmami/wn/issues/151#issuecomment-962550096>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAIPZRVZR6QDQUA3T2FW7MLUKYE3FANCNFSM5HPJT54Q>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>


-- 
Francis Bond <http://www3.ntu.edu.sg/home/fcbond/>
Division of Linguistics and Multilingual Studies
Nanyang Technological University
@goodmami: > I was indeed thinking of warning during validation.

Ok, good. It wasn't clear, so I changed the title. We could also check for similar whitespace issues in other elements like `<ILIDefinition>`, `<Count>`, `<Tag>`, and `<Pronunciation>`, or in attribute values, like for `writtenForm` or `subcategorizationFrame`.

> I think returning \n\t\t\n\t\t\n for the definition of a synset, rather
than None, is less correct and does make it less usable.  However, as you
say, the ideal time to catch this is when the wordnet is made, not when we
load.

Right. It's less correct for the language, but it's an accurate representation of what's in the data. I don't think Wn should be deciding what it thinks a language should look like. The data should do that.@francis-dion: My understanding is that, in XML, white space after the opening tag and before the closing tag should be ignored.
I didn't trace the original specs, but found multiple references including this one from adobe:
`XML ignores the first sequence of white space immediately after the opening tag and the last sequence of white space immediately before the closing tag. XML translates non-space characters (tab and new-line) into a space character and consolidates all multiple space characters into a single space` 

If the author of a wordnet wants/needs white space preserved, they should use the xml:space attribute. Here's a quote from O'Reilly's xml pocket reference:
`When xml:space is used on an element with a value of preserve , the whitespace in that element's content must be preserved as is by the application that processes it. The whitespace is always passed on to the processing application, but xml:space provides the application with a hint regarding how to process it.`

Otherwise, I believe leading/trailing white space should definitively be stripped. I also think (albeit less strongly :-) that wn should be translating non-space characters (tab and new-line) into a space character and consolidate all multiple space characters into a single space.@goodmami: Thanks, @francis-dion, that's a good point. I'd forgotten about `xml:space`. The [W3 spec says this](https://www.w3.org/TR/xml/#sec-white-space) about the default value:

> The value "default" signals that applications' default white-space processing modes are acceptable for this element; the value "preserve" indicates the intent that applications preserve all the white space.

So when `xml:space` is not specified, it's not that the spacing should be stripped, but that the application should use its default whitespace processsing. So, yes, Wn could strip (and normalize) whitespace if `xml:space` is not present. One issue is if a wordnet author wishes to preserve whitespace. Obviously the answer is to use `xml:space` on the element, but the WN-LMF spec needs to declare the attribute for it to be used. From the same W3 spec:

> In valid documents, this attribute, like any other, MUST be [declared](https://www.w3.org/TR/xml/#dt-attdecl) if it is used.@goodmami: The `xml:space` attribute has been added to WN-LMF 1.3 and if it is not `xml:space="preserve"`, whitespace is normalized in element text, as of Wn v0.10.0. What remains of this issue is to validate for empty definitions/examples.

While we're at it, it would be good to check for duplicate definitions (see https://github.com/globalwordnet/english-wordnet/issues/1134)

Solution patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index cd2021f..0907b39 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,9 @@
 * `Relation` class ([#216])
 * `Sense.relation_map()` method ([#216])
 * `Synset.relation_map()` method ([#167], [#216])
+* `W305` blank definition on synset validation ([#151])
+* `W306` blank example on synset validation ([#151])
+* `W307` repeated definition on synset validation ([#151])
 
 ## Fixed
 
@@ -687,6 +690,7 @@ abandoned, but this is an entirely new codebase.
 [#146]: https://github.com/goodmami/wn/issues/146
 [#147]: https://github.com/goodmami/wn/issues/147
 [#148]: https://github.com/goodmami/wn/issues/148
+[#151]: https://github.com/goodmami/wn/issues/151
 [#152]: https://github.com/goodmami/wn/issues/152
 [#154]: https://github.com/goodmami/wn/issues/154
 [#155]: https://github.com/goodmami/wn/issues/155
diff --git a/wn/validate.py b/wn/validate.py
index 9e9dd4b..2d867c6 100644
--- a/wn/validate.py
+++ b/wn/validate.py
@@ -16,6 +16,9 @@ W301  Synset is empty (not associated with any lexical entries).
 W302  ILI is repeated across synsets.
 W303  Proposed ILI is missing a definition.
 W304  Existing ILI has a spurious definition.
+W305  Synset has a blank definition.
+W306  Synset has a blank example.
+W307  Synset repeats an existing definition.
 E401  Relation target is missing or invalid.
 W402  Relation type is invalid for the source and target.
 W403  Redundant relation between source and target.
@@ -125,6 +128,34 @@ def _spurious_ili_definition(lex: lmf.Lexicon, ids: _Ids) -> _Result:
             if ss['ili'] and ss['ili'] != 'in' and ss.get('ili_definition')}
 
 
+def _blank_synset_definition(lex: lmf.Lexicon, ids: _Ids) -> _Result:
+    """synset has a blank definition"""
+    return {
+        ss['id']: {} for ss in _synsets(lex)
+        if any(dfn["text"].strip() == "" for dfn in ss.get("definitions", []))
+    }
+
+def _blank_synset_example(lex: lmf.Lexicon, ids: _Ids) -> _Result:
+    """synset has a blank example"""
+    return {
+        ss['id']: {} for ss in _synsets(lex)
+        if any(ex["text"].strip() == "" for ex in ss.get("examples", []))
+    }
+
+
+def _repeated_synset_definition(lex: lmf.Lexicon, ids: _Ids) -> _Result:
+    """synset repeats an existing definition"""
+    repeated = _multiples(
+        dfn["text"]
+        for ss in _synsets(lex)
+        for dfn in ss.get("definitions", [])
+    )
+    return {
+        ss["id"]: {} for ss in _synsets(lex)
+        if any(dfn["text"] in repeated for dfn in ss.get("definitions", []))
+    }
+
+
 def _missing_relation_target(lex: lmf.Lexicon, ids: _Ids) -> _Result:
     """relation target is missing or invalid"""
     result = {s['id']: {'type': r['relType'], 'target': r['target']}
@@ -253,6 +284,9 @@ _codes: dict[str, _CheckFunction] = {
     'W302': _repeated_ili,
     'W303': _missing_ili_definition,
     'W304': _spurious_ili_definition,
+    'W305': _blank_synset_definition,
+    'W306': _blank_synset_example,
+    'W307': _repeated_synset_definition,
     # 400 - relations
     'E401': _missing_relation_target,
     'W402': _invalid_relation_type,


Test patch:
diff --git a/tests/data/W305-0.xml b/tests/data/W305-0.xml
new file mode 100644
index 0000000..3a6b2df
--- /dev/null
+++ b/tests/data/W305-0.xml
@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE LexicalResource SYSTEM "http://globalwordnet.github.io/schemas/WN-LMF-1.0.dtd">
+<LexicalResource xmlns:dc="http://purl.org/dc/elements/1.1/">
+
+<!-- blank definition in synset -->
+
+  <Lexicon id="test-w305"
+           label="Testing W305"
+           language="en"
+           email="maintainer@example.com"
+           license="https://creativecommons.org/licenses/by/4.0/"
+           version="1">
+
+    <LexicalEntry id="test-w305-foo-n">
+      <Lemma partOfSpeech="n" writtenForm="foo" />
+      <Sense id="test-w305-foo-n" synset="test-w305-01-n" />
+    </LexicalEntry>
+
+    <Synset id="test-w305-01-n" ili="i12345" partOfSpeech="n">
+      <Definition>
+        
+      </Definition>
+    </Synset>
+
+  </Lexicon>
+
+</LexicalResource>
diff --git a/tests/data/W306-0.xml b/tests/data/W306-0.xml
new file mode 100644
index 0000000..1aae60b
--- /dev/null
+++ b/tests/data/W306-0.xml
@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE LexicalResource SYSTEM "http://globalwordnet.github.io/schemas/WN-LMF-1.0.dtd">
+<LexicalResource xmlns:dc="http://purl.org/dc/elements/1.1/">
+
+<!-- blank example in synset -->
+
+  <Lexicon id="test-w306"
+           label="Testing W306"
+           language="en"
+           email="maintainer@example.com"
+           license="https://creativecommons.org/licenses/by/4.0/"
+           version="1">
+
+    <LexicalEntry id="test-w306-foo-n">
+      <Lemma partOfSpeech="n" writtenForm="foo" />
+      <Sense id="test-w306-foo-n" synset="test-w306-01-n" />
+    </LexicalEntry>
+
+    <Synset id="test-w306-01-n" ili="i12345" partOfSpeech="n">
+      <Example>
+        
+      </Example>
+    </Synset>
+
+  </Lexicon>
+
+</LexicalResource>
diff --git a/tests/data/W307-0.xml b/tests/data/W307-0.xml
new file mode 100644
index 0000000..61f39e7
--- /dev/null
+++ b/tests/data/W307-0.xml
@@ -0,0 +1,30 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE LexicalResource SYSTEM "http://globalwordnet.github.io/schemas/WN-LMF-1.0.dtd">
+<LexicalResource xmlns:dc="http://purl.org/dc/elements/1.1/">
+
+<!-- repeated definition in synset -->
+
+  <Lexicon id="test-w307"
+           label="Testing W307"
+           language="en"
+           email="maintainer@example.com"
+           license="https://creativecommons.org/licenses/by/4.0/"
+           version="1">
+
+    <LexicalEntry id="test-w307-foo-n">
+      <Lemma partOfSpeech="n" writtenForm="foo" />
+      <Sense id="test-w307-foo-1-n" synset="test-w307-01-n" />
+      <Sense id="test-w307-foo-2-n" synset="test-w307-02-n" />
+    </LexicalEntry>
+
+    <Synset id="test-w307-01-n" ili="i12345" partOfSpeech="n">
+      <Definition>foo</Definition>
+    </Synset>
+
+    <Synset id="test-w307-02-n" ili="i12346" partOfSpeech="n">
+      <Definition>foo</Definition>
+    </Synset>
+
+  </Lexicon>
+
+</LexicalResource>
diff --git a/tests/validate_test.py b/tests/validate_test.py
index 76935db..8509600 100644
--- a/tests/validate_test.py
+++ b/tests/validate_test.py
@@ -1,18 +1,24 @@
-from pathlib import Path
+import pytest
 
 from wn import lmf
 from wn.validate import validate
 
+tests = [
+    ("E101", 0),
+    ("E101", 1),
+    ("E101", 2),
+    ("E101", 3),
+    ("W305", 0),
+    ("W306", 0),
+    ("W307", 0),
+]
+test_ids = [f"{code}-{i}" for code, i in tests]
 
-def _assert_invalid(select: str, path: Path) -> None:
+
+@pytest.mark.parametrize("code,i", tests, ids=test_ids)
+def test_validate(datadir, code: str, i: int) -> None:
+    path = datadir / f"{code}-{i}.xml"
     lex = lmf.load(path, progress_handler=None)["lexicons"][0]
-    report = validate(lex, select=[select], progress_handler=None)
+    report = validate(lex, select=[code], progress_handler=None)
     print(report)
-    assert len(report[select]["items"]) > 0
-
-
-def test_E101(datadir):
-    _assert_invalid("E101", datadir / "E101-0.xml")
-    _assert_invalid("E101", datadir / "E101-1.xml")
-    _assert_invalid("E101", datadir / "E101-2.xml")
-    _assert_invalid("E101", datadir / "E101-3.xml")
+    assert len(report[code]["items"]) > 0