Small fixes and typos in related to filter's grammar

[fekad]

This PR contains a few small fixes related to filter's grammar which were partly discussed in an ongoing PR (Materials-Consortia/optimade-python-tools#66). Thanks for the help of @sauliusg, these fixes have been pushed forward to merge into the main repository.

[CasperWA]

Thanks @fekad!

Most of these changes (in the spec) seem to be format fixes, concerning rst.
However, there are some content changes as well. To me they look completely fine, but I am not a grammar expert and would refer to @sauliusg, @rartino, @merkys or others to verify the correctness of the content. Although it seems it has already been through the meticulous mind of @sauliusg 😉

Concerning the many .out files, are these changed manually or automatically? I hope the latter.
It was more to ask also if they should be reviewed or can be taken as granted?

[fekad]

Concerning the many .out files, are these changed manually or automatically? I hope the latter.
It was more to ask also if they should be reviewed or can be taken as granted?

Thanks, @CasperWA! So all of the .out files have been patched by using the diff files which were generated during the test itself. I know it's cheating :) but I checked all of them manually.

[rartino]

These changes are ok with me;

Except, why do you want to collapse 'PredicateComparison'? Human readability is also important for a grammar, and looking at Comparison | LengthComparison will have anyone wondering why a LengthComparison isn't a Comparison, whereas with Comparison | PredicateComparison the division is more clear.

Also, at the point we introduce more predicates, we'd have to reintroduce that layer and everyone with parsers will need to update that part.

(However, I prefer @sauliusg also looking at these changes if he has the time, so I'll hold on my approval so this isn't merged yet.)

[sauliusg]

These changes are ok with me;

Except, why do you want to collapse 'PredicateComparison'? Human readability is also important for a grammar, and looking at Comparison | LengthComparison will have anyone wondering why a LengthComparison isn't a Comparison, whereas with Comparison | PredicateComparison the division is more clear.

Also, at the point we introduce more predicates, we'd have to reintroduce that layer and everyone with parsers will need to update that part.

(However, I prefer @sauliusg also looking at these changes if he has the time, so I'll hold on my approval so this isn't merged yet.)

My previous comment was in favour of unifying PredicateComparison and LengthComparison since they are defined to be equal; I don't think having two names for the same concept helps with clarity, rather the opposite... The point is that we use one term in the definition, and another term at the point when it is used, and the reader must always keep in mind the relation between the two, which in this case is just an identity. Rather confusing.

Maybe, however, we should leave 'PredicateComparison', defining it as:

PredicateComparison = LENGTH, Property, Operator, Value ;

and drop LengthComparison?

It would then have the clarity at the point of use as desired by @rartino and would not have the second name definition (as @fekad and myself would agree to see it). That the PredicteComparison currently has just the LENGTH predicate is evident at the point of definition.

If we all agree on this change I would be happy to see this PR merged.

[sauliusg]

Also, at the point we introduce more predicates, we'd have to reintroduce that layer and everyone with parsers will need to update that part.

I agree, but OTOH we will not be able to anticipate every possible change. And having a name "just in case" does not seem right, it is analogous to "dead code" and I would considered it bad in general. If we need a new grammar construct we can always introduce it later, at the point when it is actually functional, can't we?

[sauliusg]

Maybe, however, we should leave 'PredicateComparison', defining it as:

PredicateComparison = LENGTH, Property, Operator, Value ;

and drop LengthComparison?

It would then have the clarity at the point of use as desired by @rartino and would not have the second name definition (as @fekad and myself would agree to see it). That the PredicteComparison currently has just the LENGTH predicate is evident at the point of definition.

If we all agree on this change I would be happy to see this PR merged.

[rartino]

Alright, I'm (somewhat reluctantly) ok with this.

But, I rather see a PR that implements the end agreement (?) in #199, which just gets rid of the predicate form.

[CasperWA]

But, I rather see a PR that implements the end agreement (?) in #199, which just gets rid of the predicate form.

Just to clarify, you'd rather change PredicateComparison back to LengthComparison and then reintroduce PredicateComparison in the future if it should be needed?

[fekad]

I reverted all the commits related to PredicateComparison just to simplify this PR. We can easily create a new PR once we have any decisions on #199 or #198

[rartino]

Just to clarify, you'd rather change PredicateComparison back to LengthComparison and then reintroduce PredicateComparison in the future if it should be needed?

My main preference is to follow the present suggestion in #199 and change the LENGTH syntax to

list LENGTH 3

which completely drops predicates from the language. At that point, PredicateComparison wouldn't remain at all and I would have a preference to push further language additions to non-predicate forms.

[rartino]

This version looks good to me. Thanks for the effort with the syntax cleanup.

[CasperWA]

Thanks @fekad for this contribution, and @sauliusg, @rartino for the grammar-specific inputs and review.

The matter of PredicateComparison is reverted, and issue #199 may be further addressed in (a) future PR(s).