Refining the grammar of the `LengthComparison` #221

fekad · 2019-12-03T19:59:55Z

Closes #199

This PR has been made according to a previous issue (#199).

rartino

I really like this PR, because it simplifies the language. I want this PR to be in 1.0, because to go back and change something like this after we've released 1.0 is a horrible idea.

However, before I click 'approve', I want more people who are currently engaged in writing implementations to say that they agree, since this does force them to go back and change things that are already working for a mere syntax improvement.

I'm going to 'request review' on a few of them.

optimade.rst

tests/inputs/length_fail.filter

merkys

To me this seems OK, and I'd be happy to have it merged ASAP.

rartino

(I'm making a blocking "request changes" review here, as I will urge others to approve the change and want to avoid accidental merging the changes until a few of those are in.)

CasperWA

Looks good to me (as long as we ASAP update the grammar in optimade-python-tools 😉 ).
Thanks @fekad!

optimade.rst

giovannipizzi · 2019-12-04T21:26:37Z

For me the change to the grammar is ok to be made - I'm not approving since anyway I see there are other changes requested, but happy to do if needed when all changes have been taken care of.

rartino · 2019-12-05T18:51:16Z

I think enough time has passed without protest to approve this. @fekad, can you please take a look at the suggested edits? At the very least, I agree with @CasperWA that it needs to be said somewhere in the specification that when the operator is omitted, LENGTH tests for equality.

CasperWA

Some clarifications desired. Mainly for the sentence I put in a suggested change for. Otherwise, I would say this is good-to-go.

optimade.rst

Co-Authored-By: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

rartino

One reformulation, otherwise good to go.

optimade.rst

Co-Authored-By: Rickard Armiento <gitcommits@armiento.net>

CasperWA

Cheers @fekad. And thanks for patiently responding to our (my) many questions 👍

rartino

Great!

…/OPTIMADE#221.

Update from v0.10.0: Implement Materials-Consortia/OPTIMADE#221

Adam Fekete added 4 commits November 27, 2019 18:36

adding LengthOpRhs to the grammar

a89254e

using the Value instead of the StringProperty

1c9a269

updating appearance of LENGTH and Value

daa0948

fixing test input

6ff2e2f

fekad mentioned this pull request Dec 3, 2019

Refining the grammar of the LengthComparison #199

Closed

fekad added the PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing label Dec 3, 2019

rartino reviewed Dec 3, 2019

View reviewed changes

optimade.rst Outdated Show resolved Hide resolved

tests/inputs/length_fail.filter Outdated Show resolved Hide resolved

rartino requested review from merkys and CasperWA December 3, 2019 22:23

merkys previously approved these changes Dec 4, 2019

View reviewed changes

rartino requested changes Dec 4, 2019

View reviewed changes

CasperWA previously approved these changes Dec 4, 2019

View reviewed changes

optimade.rst Show resolved Hide resolved

optimade.rst Outdated Show resolved Hide resolved

rartino added PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR and removed PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing labels Dec 5, 2019

minor fixes + clarifying the usage of ops in the LENGTH filter

895bc00

fekad dismissed stale reviews from CasperWA and merkys via 895bc00 December 6, 2019 13:23

fekad requested review from rartino, merkys and CasperWA December 6, 2019 15:05

merkys previously approved these changes Dec 6, 2019

View reviewed changes

CasperWA added PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing and removed PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR labels Dec 8, 2019

CasperWA requested changes Dec 8, 2019

View reviewed changes

optimade.rst Outdated Show resolved Hide resolved

optimade.rst Outdated Show resolved Hide resolved

CasperWA added PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR and removed PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing labels Dec 8, 2019

reverting unclear definition

c89ba0b

Co-Authored-By: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

fekad dismissed merkys’s stale review via c89ba0b December 9, 2019 15:08

rartino requested changes Dec 10, 2019

View reviewed changes

optimade.rst Outdated Show resolved Hide resolved

'matches if' definition of LENGTH

87fa005

Co-Authored-By: Rickard Armiento <gitcommits@armiento.net>

fekad added PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing and removed PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR labels Dec 10, 2019

CasperWA approved these changes Dec 11, 2019

View reviewed changes

CasperWA requested review from rartino and merkys December 11, 2019 17:36

rartino approved these changes Dec 11, 2019

View reviewed changes

rartino merged commit 4c6c865 into develop Dec 11, 2019

merkys deleted the issue_199_length_comarison branch December 12, 2019 07:19

merkys added a commit to Materials-Consortia/OPTIMADE-Filter that referenced this pull request Dec 12, 2019

Changing the syntax of 'LENGTH' comparison as per Materials-Consortia…

45462bd

…/OPTIMADE#221.

CasperWA added a commit to CasperWA/optimade-python-tools that referenced this pull request Dec 12, 2019

Introduce grammar v0.10.1

ded3b52

Update from v0.10.0: Implement Materials-Consortia/OPTIMADE#221

CasperWA mentioned this pull request Dec 12, 2019

Introduce grammar v0.10.1 Materials-Consortia/optimade-python-tools#112

Merged

CasperWA mentioned this pull request Jan 6, 2020

Minor changes to specification v0.10.1-develop Materials-Consortia/optimade-python-tools#115

Closed

9 tasks

CasperWA added a commit to Materials-Consortia/optimade-python-tools that referenced this pull request Jan 7, 2020

Introduce grammar v0.10.1

b16af72

Update from v0.10.0: Implement Materials-Consortia/OPTIMADE#221

CasperWA added a commit to Materials-Consortia/optimade-python-tools that referenced this pull request Jan 8, 2020

Introduce grammar v0.10.1

179a2a6

Update from v0.10.0: Implement Materials-Consortia/OPTIMADE#221

CasperWA added a commit to Materials-Consortia/optimade-python-tools that referenced this pull request Jan 8, 2020

Introduce grammar v0.10.1

98c0879

Update from v0.10.0: Implement Materials-Consortia/OPTIMADE#221

CasperWA added a commit to Materials-Consortia/optimade-python-tools that referenced this pull request Jan 9, 2020

Introduce grammar v0.10.1

b3a36b0

Update from v0.10.0: Implement Materials-Consortia/OPTIMADE#221

CasperWA added a commit to Materials-Consortia/optimade-python-tools that referenced this pull request Jan 10, 2020

Introduce grammar v0.10.1

2407fc4

Update from v0.10.0: Implement Materials-Consortia/OPTIMADE#221

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refining the grammar of the `LengthComparison` #221

Refining the grammar of the `LengthComparison` #221

fekad commented Dec 3, 2019 •

edited by CasperWA

rartino left a comment •

edited

merkys left a comment

rartino left a comment •

edited

CasperWA left a comment

giovannipizzi commented Dec 4, 2019

rartino commented Dec 5, 2019

CasperWA left a comment

rartino left a comment

CasperWA left a comment

rartino left a comment

Refining the grammar of the LengthComparison #221

Refining the grammar of the LengthComparison #221

Conversation

fekad commented Dec 3, 2019 • edited by CasperWA

rartino left a comment • edited

Choose a reason for hiding this comment

merkys left a comment

Choose a reason for hiding this comment

rartino left a comment • edited

Choose a reason for hiding this comment

CasperWA left a comment

Choose a reason for hiding this comment

giovannipizzi commented Dec 4, 2019

rartino commented Dec 5, 2019

CasperWA left a comment

Choose a reason for hiding this comment

rartino left a comment

Choose a reason for hiding this comment

CasperWA left a comment

Choose a reason for hiding this comment

rartino left a comment

Choose a reason for hiding this comment

Refining the grammar of the `LengthComparison` #221

Refining the grammar of the `LengthComparison` #221

fekad commented Dec 3, 2019 •

edited by CasperWA

rartino left a comment •

edited

rartino left a comment •

edited