Repository: Materials-Consortia/optimade-python-tools
Pull request #879 (2021-07-20 17:08:48)

Tidy up old grammars, add a development grammar for v1.2 and update filterparser tests
This PR does the following:

- removes pre-1.0 grammars and their tests
- Adds a v1.2 grammar that includes `TRUE` and `FALSE` tokens,
- Adds links to a v1.1.0 and v1.0.1 grammars based on v1.0.0
- Unifies the testing of grammars such that each new grammar version inherits the tests from previous versions

codecov[bot]: # [Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/879?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia) Report
> Merging [#879](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/879?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia) (ae51874) into [master](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/commit/1f7ec8a96b4f9f93f3d4b3695aac2245851c33b1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia) (1f7ec8a) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/879/graphs/tree.svg?width=650&height=150&src=pr&token=UJAtmqkZZO&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia)](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/879?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia)

```diff
@@           Coverage Diff           @@
##           master     #879   +/-   ##
=======================================
  Coverage   92.74%   92.74%           
=======================================
  Files          67       67           
  Lines        3763     3764    +1     
=======================================
+ Hits         3490     3491    +1     
  Misses        273      273           
```

| Flag | Coverage Δ | |
|---|---|---|
| project | `92.74% <100.00%> (+<0.01%)` | :arrow_up: |
| validator | `92.74% <100.00%> (+<0.01%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/879?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia) | Coverage Δ | |
|---|---|---|
| [optimade/filterparser/lark\_parser.py](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/879/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia#diff-b3B0aW1hZGUvZmlsdGVycGFyc2VyL2xhcmtfcGFyc2VyLnB5) | `95.12% <100.00%> (+0.12%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/879?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/879?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia). Last update [1f7ec8a...ae51874](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/879?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Materials-Consortia).

ml-evs: > There seem to have been added a lot of files in the folder testfiles. Was this intentional, or are these temporary files that should be removed? Never mind, I had read only the initial version of your message at the start of this PR.

I think there's probably a better way of doing this so I will convert to a draft PR for now. Perhaps an `invoke` task that takes the examples from the spec repo and condenses them into one file with some tags for the examples which are expected to fail.
ml-evs: Also apologies @markus1978, this PR touches one line of the elastic tests so you ended up getting pinged as a reviewer, feel free to ignore.
ml-evs: Thought I'd have a bit more time to extend this, but its probably useful as-is. The tests still only check whether parsing does "something", not necessarily the right thing, but we "know" the grammar works so I can't justify spending more time on these tests.

Patch:
diff --git a/optimade/filterparser/lark_parser.py b/optimade/filterparser/lark_parser.py
index f1200ed0..ab327602 100644
--- a/optimade/filterparser/lark_parser.py
+++ b/optimade/filterparser/lark_parser.py
@@ -61,7 +61,10 @@ class LarkParser:
 
         """
 
-        version = version if version else max(AVAILABLE_PARSERS.keys())
+        if not version:
+            version = max(
+                _ for _ in AVAILABLE_PARSERS if AVAILABLE_PARSERS[_].get("default")
+            )
 
         if version not in AVAILABLE_PARSERS:
             raise ParserError(f"Unknown parser grammar version: {version}")
diff --git a/optimade/grammar/v0.9.5.lark b/optimade/grammar/v0.9.5.lark
deleted file mode 100644
index 4ca776c9..00000000
--- a/optimade/grammar/v0.9.5.lark
+++ /dev/null
@@ -1,28 +0,0 @@
-// optimade v0.9.5 grammar spec in lark grammar format
-
-start: KEYWORD expression
-KEYWORD: "filter="
-expression: [expression OR] term
-term: [term AND] atom
-atom: [NOT] comparison
-    | [NOT] "(" (andcomparison OR)* andcomparison ")"
-andcomparison: [NOT] (NOT comparison AND)* comparison
-comparison: VALUE OPERATOR VALUE
-OPERATOR: /<=?|>=?|!?=/
-VALUE: CNAME | SIGNED_FLOAT | SIGNED_INT | ESCAPED_STRING
-AND: /and/i
-OR: /or/i
-NOT: /not/i
-
-// Strings
-
-_STRING_INNER: /(.|[\t\f\r\n])*?/
-_STRING_ESC_INNER: _STRING_INNER /(?<!\\)(\\\\)*?/
-
-ESCAPED_STRING : "\"" _STRING_ESC_INNER "\""
-
-%import common.CNAME
-%import common.SIGNED_FLOAT
-%import common.SIGNED_INT
-%import common.WS_INLINE
-%ignore WS_INLINE
diff --git a/optimade/grammar/v0.9.6.lark b/optimade/grammar/v0.9.6.lark
deleted file mode 100644
index fde92c58..00000000
--- a/optimade/grammar/v0.9.6.lark
+++ /dev/null
@@ -1,33 +0,0 @@
-// optimade v0.9.6 grammar spec in lark grammar format
-
-start: KEYWORD expression
-KEYWORD: "filter=" | "filter ="
-expression: [expression CONJUNCTION] term
-term: [term CONJUNCTION] atom | "(" [term CONJUNCTION] term
-
-atom: [NOT] comparison
-
-comparison: VALUE OPERATOR VALUE [")"] | VALUE OPERATOR "'" (combined)* "'"
-OPERATOR: /<=?|>=?|!?=/
-
-combined: VALUE ", " | VALUE "," | VALUE
-
-VALUE: CNAME | SIGNED_FLOAT | SIGNED_INT | ESCAPED_STRING
-
-CONJUNCTION: AND | OR
-AND: /and/i
-OR: /or/i
-NOT: /not/i
-
-// Strings
-
-_STRING_INNER: /(.|[\t\f\r\n])*?/
-_STRING_ESC_INNER: _STRING_INNER /(?<!\\)(\\\\)*?/
-
-ESCAPED_STRING : "\"" _STRING_ESC_INNER "\""
-
-%import common.CNAME
-%import common.SIGNED_FLOAT
-%import common.SIGNED_INT
-%import common.WS_INLINE
-%ignore WS_INLINE
diff --git a/optimade/grammar/v0.9.7.lark b/optimade/grammar/v0.9.7.lark
deleted file mode 100644
index 8daeea41..00000000
--- a/optimade/grammar/v0.9.7.lark
+++ /dev/null
@@ -1,34 +0,0 @@
-// optimade v0.9.7 grammar spec in lark grammar format
-// CHANGED start to not require KEYWORD filter=
-
-start: expression
-expression: [expression CONJUNCTION] term
-term: [term CONJUNCTION] atom | "(" [term CONJUNCTION] term ")"
-
-atom: [NOT] comparison
-
-comparison: VALUE OPERATOR VALUE | VALUE OPERATOR "'" combined "'"
-
-OPERATOR: /<=?|>=?|!?=/
-
-combined: (VALUE ",")* VALUE
-
-VALUE: CNAME | SIGNED_FLOAT | SIGNED_INT | ESCAPED_STRING
-
-CONJUNCTION: AND | OR
-AND: /and/i
-OR: /or/i
-NOT: /not/i
-
-// Strings
-
-_STRING_INNER: /(.|[\t\f\r\n])*?/
-_STRING_ESC_INNER: _STRING_INNER /(?<!\\)(\\\\)*?/
-
-ESCAPED_STRING : "\"" _STRING_ESC_INNER "\""
-
-%import common.CNAME
-%import common.SIGNED_FLOAT
-%import common.SIGNED_INT
-%import common.WS_INLINE
-%ignore WS_INLINE
diff --git a/optimade/grammar/v0.10.1.lark b/optimade/grammar/v1.0.1.lark
similarity index 100%
rename from optimade/grammar/v0.10.1.lark
rename to optimade/grammar/v1.0.1.lark
diff --git a/optimade/grammar/v1.1.0.lark b/optimade/grammar/v1.1.0.lark
new file mode 120000
index 00000000..c5582225
--- /dev/null
+++ b/optimade/grammar/v1.1.0.lark
@@ -0,0 +1,1 @@
+v1.0.0.lark
\ No newline at end of file
diff --git a/optimade/grammar/v0.10.0.lark b/optimade/grammar/v1.2.0.develop.lark
similarity index 74%
rename from optimade/grammar/v0.10.0.lark
rename to optimade/grammar/v1.2.0.develop.lark
index 7a817514..12ec22c0 100644
--- a/optimade/grammar/v0.10.0.lark
+++ b/optimade/grammar/v1.2.0.develop.lark
@@ -1,4 +1,4 @@
-// optimade v0.10.0 grammar spec in lark grammar format
+// optimade v1.2.0 (expected version) grammar spec in lark grammar format
 
 ?start: filter
 filter: expression*
@@ -6,7 +6,11 @@ filter: expression*
 // Values
 constant: string | number
 // Note: support for property in value is OPTIONAL
-value: string | number | property
+value: string | bool | number | property
+
+// Note: not_implemented_string is only here to help Transformers
+non_string_value: number | property
+not_implemented_string: string
 
 // Note:  support for OPERATOR in value_list is OPTIONAL
 value_list: [ OPERATOR ] value ( "," [ OPERATOR ] value )*
@@ -17,7 +21,7 @@ value_zip_list: value_zip ( "," value_zip )*
 // Expressions
 expression: expression_clause ( _OR expression_clause )*
 expression_clause: expression_phrase ( _AND expression_phrase )*
-expression_phrase: [ NOT ] ( comparison | predicate_comparison | "(" expression ")" )
+expression_phrase: [ NOT ] ( comparison | "(" expression ")" )
 // Note: support for constant_first_comparison is OPTIONAL
 comparison: constant_first_comparison | property_first_comparison
 
@@ -26,15 +30,16 @@ property_first_comparison: property ( value_op_rhs
                                     | known_op_rhs
                                     | fuzzy_string_op_rhs
                                     | set_op_rhs
-                                    | set_zip_op_rhs )
+                                    | set_zip_op_rhs
+                                    | length_op_rhs )
 
-constant_first_comparison: constant value_op_rhs
+constant_first_comparison: constant OPERATOR ( non_string_value | not_implemented_string )
 
 value_op_rhs: OPERATOR value
 known_op_rhs: IS ( KNOWN | UNKNOWN )
-fuzzy_string_op_rhs: CONTAINS string
-                   | STARTS [ WITH ] string
-                   | ENDS [ WITH ] string
+fuzzy_string_op_rhs: CONTAINS value
+                   | STARTS [ WITH ] value
+                   | ENDS [ WITH ] value
 // Note: support for ONLY in set_op_rhs is OPTIONAL
 // Note: support for [ OPERATOR ] in set_op_rhs is OPTIONAL
 // set_op_rhs: HAS [ ALL | ANY | ONLY] value_list
@@ -43,8 +48,10 @@ set_op_rhs: HAS ( [ OPERATOR ] value
                  | ANY value_list
                  | ONLY value_list )
 
+// Note: support for [ OPERATOR ] is OPTIONAL
+length_op_rhs: LENGTH [ OPERATOR ] signed_int
+
 set_zip_op_rhs: property_zip_addon HAS ( value_zip | ONLY value_zip_list | ALL value_zip_list | ANY value_zip_list )
-predicate_comparison: LENGTH property OPERATOR value
 property_zip_addon: ":" property (":" property)*
 
 // Property syntax
@@ -53,9 +60,15 @@ property: IDENTIFIER ( "." IDENTIFIER )*
 // String syntax
 string: ESCAPED_STRING
 
+// Bool token syntax
+bool: ( "TRUE" | "FALSE" )
+
 // Number token syntax
 number: SIGNED_INT | SIGNED_FLOAT
 
+// Custom signed int
+signed_int: SIGNED_INT
+
 // Tokens
 
 // Boolean relations
@@ -92,7 +105,6 @@ _STRING_ESC_INNER: _STRING_INNER /(?<!\\)(\\\\)*?/
 
 ESCAPED_STRING : "\"" _STRING_ESC_INNER "\""
 
-
 %import common.SIGNED_INT
 %import common.SIGNED_FLOAT
 


Test patch:
diff --git a/tests/filterparser/test_filterparser.py b/tests/filterparser/test_filterparser.py
index 913fc707..99007b9d 100644
--- a/tests/filterparser/test_filterparser.py
+++ b/tests/filterparser/test_filterparser.py
@@ -1,5 +1,5 @@
-import os
-from glob import glob
+import abc
+from typing import Tuple
 
 import pytest
 
@@ -8,49 +8,35 @@ from lark import Tree
 from optimade.filterparser import LarkParser
 from optimade.server.exceptions import BadRequest
 
-testfile_dir = os.path.join(os.path.dirname(__file__), "testfiles")
 
+class BaseTestFilterParser(abc.ABC):
+    """Base class for parsing different versions of the grammar using `LarkParser`."""
+
+    version: Tuple[int, int, int]
+    variant: str = "default"
 
-class TestParserV0_9_5:
     @pytest.fixture(autouse=True)
     def set_up(self):
-        self.test_filters = []
-        for fn in sorted(glob(os.path.join(testfile_dir, "*.inp"))):
-            with open(fn) as f:
-                self.test_filters.append(f.read().strip())
-        self.parser = LarkParser(version=(0, 9, 5))
-
-    def test_inputs(self):
-        for tf in self.test_filters:
-            if tf == "filter=number=0.0.1":
-                with pytest.raises(BadRequest):
-                    self.parser.parse(tf)
-            else:
-                tree = self.parser.parse(tf)
-                assert isinstance(tree, Tree)
-
-    def test_parser_version(self):
-        v = (0, 9, 5)
-        p = LarkParser(version=v)
-        assert isinstance(p.parse(self.test_filters[0]), Tree)
-        assert p.version == v
+        self.parser = LarkParser(version=self.version, variant=self.variant)
 
     def test_repr(self):
         assert repr(self.parser) is not None
-        self.parser.parse(self.test_filters[0])
+        self.parse("band_gap = 1")
         assert repr(self.parser) is not None
 
+    def parse(self, inp):
+        return self.parser.parse(inp)
 
-class TestParserV1_0_0:
-    version = (1, 0, 0)
-    variant = "default"
+    def test_parser_version(self):
+        assert self.parser.version == self.version
+        assert self.parser.variant == self.variant
 
-    @pytest.fixture(autouse=True)
-    def set_up(self):
-        self.parser = LarkParser(version=self.version, variant=self.variant)
 
-    def parse(self, inp):
-        return self.parser.parse(inp)
+class TestParserV1_0_0(BaseTestFilterParser):
+    """Test cases for the v1.0.0 stable release grammar."""
+
+    version = (1, 0, 0)
+    variant = "default"
 
     def test_empty(self):
         assert isinstance(self.parse(" "), Tree)
@@ -276,11 +262,34 @@ class TestParserV1_0_0:
         with pytest.raises(BadRequest):
             self.parse("NOTICE=val")  # not valid property or value (NOTICE)
 
-    def test_parser_version(self):
-        assert self.parser.version == self.version
-        assert self.parser.variant == self.variant
 
-    def test_repr(self):
-        assert repr(self.parser) is not None
-        self.parser.parse('key="value"')
-        assert repr(self.parser) is not None
+class TestParserV1_2_0(TestParserV1_0_0):
+    """Additional tests for the v1.2.0 development grammar.
+
+    Should additionally pass all v1.0.0 tests.
+
+    """
+
+    version = (1, 2, 0)
+    variant = "develop"
+
+    def test_boolean_values(self):
+        assert isinstance(
+            self.parse("_exmpl_element_counts = TRUE"),
+            Tree,
+        )
+
+        assert isinstance(
+            self.parse("_exmpl_element_counts = FALSE"),
+            Tree,
+        )
+
+        assert isinstance(
+            self.parse("_exmpl_element_counts != FALSE"),
+            Tree,
+        )
+
+        assert isinstance(
+            self.parse("NOT _exmpl_element_counts = TRUE"),
+            Tree,
+        )
diff --git a/tests/filterparser/testfiles/parse_001.inp b/tests/filterparser/testfiles/parse_001.inp
deleted file mode 100644
index e1587e24..00000000
--- a/tests/filterparser/testfiles/parse_001.inp
+++ /dev/null
@@ -1,1 +0,0 @@
-filter=te < st
diff --git a/tests/filterparser/testfiles/parse_002.inp b/tests/filterparser/testfiles/parse_002.inp
deleted file mode 100644
index 43c1f436..00000000
--- a/tests/filterparser/testfiles/parse_002.inp
+++ /dev/null
@@ -1,1 +0,0 @@
-filter=spacegroup="P2"
diff --git a/tests/filterparser/testfiles/parse_003.inp b/tests/filterparser/testfiles/parse_003.inp
deleted file mode 100644
index e2b5634d..00000000
--- a/tests/filterparser/testfiles/parse_003.inp
+++ /dev/null
@@ -1,1 +0,0 @@
-filter=_cod_cell_volume<100.0
diff --git a/tests/filterparser/testfiles/parse_004.inp b/tests/filterparser/testfiles/parse_004.inp
deleted file mode 100644
index f1b1faef..00000000
--- a/tests/filterparser/testfiles/parse_004.inp
+++ /dev/null
@@ -1,1 +0,0 @@
-filter=_mp_bandgap > 5.0 AND _cod_molecular_weight < 350
diff --git a/tests/filterparser/testfiles/parse_005.inp b/tests/filterparser/testfiles/parse_005.inp
deleted file mode 100644
index 4df410ea..00000000
--- a/tests/filterparser/testfiles/parse_005.inp
+++ /dev/null
@@ -1,1 +0,0 @@
-filter=_cod_melting_point<300 AND nelements=4 AND elements="Si,O2"
diff --git a/tests/filterparser/testfiles/parse_006.inp b/tests/filterparser/testfiles/parse_006.inp
deleted file mode 100644
index 62a22952..00000000
--- a/tests/filterparser/testfiles/parse_006.inp
+++ /dev/null
@@ -1,1 +0,0 @@
-filter=number=0.ANDnumber=.0ANDnumber=0.0ANDnumber=+0ANDNUMBER=-0ANDnumber=0e1ANDnumber=0e-1ANDnumber=0e+1
diff --git a/tests/filterparser/testfiles/parse_007.inp b/tests/filterparser/testfiles/parse_007.inp
deleted file mode 100644
index 838d97fe..00000000
--- a/tests/filterparser/testfiles/parse_007.inp
+++ /dev/null
@@ -1,1 +0,0 @@
-filter=number=0.0.1
diff --git a/tests/filterparser/testfiles/parse_008.inp b/tests/filterparser/testfiles/parse_008.inp
deleted file mode 100644
index 3090fa4a..00000000
--- a/tests/filterparser/testfiles/parse_008.inp
+++ /dev/null
@@ -1,1 +0,0 @@
- filter=key=value
diff --git a/tests/filterparser/testfiles/parse_009.inp b/tests/filterparser/testfiles/parse_009.inp
deleted file mode 100644
index bb24c9e6..00000000
--- a/tests/filterparser/testfiles/parse_009.inp
+++ /dev/null
@@ -1,1 +0,0 @@
-filter=author=" someone "
diff --git a/tests/filterparser/testfiles/parse_010.inp b/tests/filterparser/testfiles/parse_010.inp
deleted file mode 100644
index 1062b45a..00000000
--- a/tests/filterparser/testfiles/parse_010.inp
+++ /dev/null
@@ -1,1 +0,0 @@
-filter=NOTICE=val
diff --git a/tests/filterparser/testfiles/parse_011.inp b/tests/filterparser/testfiles/parse_011.inp
deleted file mode 100644
index c5b2c164..00000000
--- a/tests/filterparser/testfiles/parse_011.inp
+++ /dev/null
@@ -1,1 +0,0 @@
-filter=author="Sąžininga Žąsis"
diff --git a/tests/filterparser/testfiles/parse_012.inp b/tests/filterparser/testfiles/parse_012.inp
deleted file mode 100644
index 39143f62..00000000
--- a/tests/filterparser/testfiles/parse_012.inp
+++ /dev/null
@@ -1,2 +0,0 @@
-filter=author="Sąžininga Žąs
-is"
diff --git a/tests/filterparser/testfiles/parse_013.inp b/tests/filterparser/testfiles/parse_013.inp
deleted file mode 100644
index a183d6b6..00000000
--- a/tests/filterparser/testfiles/parse_013.inp
+++ /dev/null
@@ -1,1 +0,0 @@
-filter= a = 12345 AND b = +12 AND c = -34 AND d = 1.2 AND e = .2E7 AND f = -.2E+7 AND g = +10.01E-10 AND h = 6.03e23 AND i = .1E1 AND j = -.1e1 AND k = 1.e-12 AND l = -.1e-12 AND m = 1000000000.E1000000000
diff --git a/tests/filterparser/testfiles/parse_014.inp b/tests/filterparser/testfiles/parse_014.inp
deleted file mode 100644
index 8019cd61..00000000
--- a/tests/filterparser/testfiles/parse_014.inp
+++ /dev/null
@@ -1,1 +0,0 @@
-filter=field = "!#$%&'()*+,-./:;<=>?@[]^`{|}~%"
diff --git a/tests/filtertransformers/test_elasticsearch.py b/tests/filtertransformers/test_elasticsearch.py
index bf72b729..b39618ee 100644
--- a/tests/filtertransformers/test_elasticsearch.py
+++ b/tests/filtertransformers/test_elasticsearch.py
@@ -13,7 +13,7 @@ from optimade.filtertransformers.elasticsearch import (
 
 @pytest.fixture
 def parser():
-    return LarkParser(version=(0, 10, 1))
+    return LarkParser()
 
 
 @pytest.fixture