Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Forbid constant <op> constant comparisons for strings #229

Merged
merged 5 commits into from
Dec 23, 2019
Merged

Forbid constant <op> constant comparisons for strings #229

merged 5 commits into from
Dec 23, 2019

Conversation

rartino
Copy link
Contributor

@rartino rartino commented Dec 21, 2019

Fixes #157.

@rartino rartino added this to the 1.0 release milestone Dec 21, 2019
@rartino rartino mentioned this pull request Dec 21, 2019
Open
ml-evs
ml-evs previously approved these changes Dec 21, 2019
View reviewed changes
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor formatting suggestions, will approve now, please re-request review if you decide to commit them

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
@rartino @ml-evs
Minor text fixes suggested in review
4ce37be 
Co-authored-by: Matthew Evans <7916000+ml-evs@users.noreply.github.com>
@rartino rartino requested a review from ml-evs December 21, 2019 22:40
@rartino
Copy link
Contributor Author

rartino commented Dec 21, 2019

Thanks @ml-evs for the review and suggested improvements! They should be implemented now.

@rartino rartino 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 21, 2019
ml-evs
ml-evs previously approved these changes Dec 22, 2019
View reviewed changes
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@CasperWA CasperWA mentioned this pull request Dec 22, 2019
Closed
CasperWA
CasperWA previously requested changes Dec 22, 2019
View reviewed changes
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I found a stray also, and then I have a suggestion, which you can take or leave.

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
@CasperWA 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 22, 2019
@rartino @CasperWA
Remove redundant also
3114ef1 
Co-Authored-By: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
@rartino rartino 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 23, 2019
merkys
merkys approved these changes Dec 23, 2019
View reviewed changes
Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I'd prefer grammar-based solution (no rule accepting string <operator> string), I understand we expect to implement string vs. string comparisons in future releases, and this is told explicitly in the specification. Therefore I approve.

@rartino rartino requested a review from ml-evs December 23, 2019 16:02
CasperWA
CasperWA approved these changes Dec 23, 2019
View reviewed changes
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing the suggested changes @rartino. And thanks for this clarification! 👍

@CasperWA CasperWA merged commit a7d915b into Materials-Consortia:develop Dec 23, 2019
This was referenced Jan 6, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merkys merkys merkys approved these changes

@CasperWA CasperWA CasperWA approved these changes

@ml-evs ml-evs Awaiting requested review from ml-evs

Assignees
No one assigned
Labels
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
Projects
None yet
Milestone
1.0 release
Development

Successfully merging this pull request may close these issues.

Ambiguity in filter for value vs. value comparisons of type string vs. string
4 participants
@rartino @ml-evs @merkys @CasperWA