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.

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

Filter predicate improvements #115

Merged
merged 1 commit into from
Aug 10, 2021
Merged

Filter predicate improvements #115

merged 1 commit into from
Aug 10, 2021

Conversation

glyn
Copy link
Collaborator

@glyn glyn commented Aug 6, 2021

  • Generalise comparisons
  • Make regex operator conform to Perl etc.
  • Add operator precedence table

Fixes #112
Fixes #113
Fixes #114

basic-expr = exist-expr / paren-expr / (neg-op paren-expr) / relation-expr
exist-expr = [neg-op] path ; path existence or non-existence
path = rel-path / json-path
rel-path = "@" *(dot-selector / index-selector)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we only supporting a single selector here, or does the *( ... ) mean multiple?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

*() means zero or more repetitions of the contents of the brackets. See section 3.6 of RFC 5234.

Comment on lines +1058 to +1060
comparable = number / string-literal / ; primitive ...
true / false / null / ; values only
path ; path value
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious, are we planning on supporting literal complex values (i.e. object / arrays) at some point later? Maybe this needs more discussion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree it needs more discussion, but let's defer such discussion for now to keep this PR nice and small.

|:--:|:--:|:--:|
| 5 | Grouping | `(...)` |
| 4 | Logical NOT | `!` |
| 3 | Relations | `==`<br>`!=`<br>`<`<br>`>`<br>`<=`<br>`>=`<br>`=~`<br>` in ` |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to have each operator on its own line? Seems like we could do with two or three per line. Just a readability question.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really. I've pushed a change which groups similar operators together on single lines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps I realise why this had one operator per line. Putting multiple on one line, separated by space or &nbsp; results in operators being wrapped in double quotes in the rendered text version (although not the HTML), thus:

            +============+===========+===================+
              | Precedence |  Operator |       Syntax      |
              |            |    type   |                   |
              +============+===========+===================+
              |     5      |  Grouping |       (...)       |
              +------------+-----------+-------------------+
              |     4      |  Logical  |         !         |
              |            |    NOT    |                   |
              +------------+-----------+-------------------+
              |     3      | Relations |     "==" "!="     |
              |            |           | "<" "<=" ">" ">=" |
              |            |           |        "=~"       |
              |            |           |        "in"       |
              +------------+-----------+-------------------+
              |     2      |  Logical  |         &&        |
              |            |    AND    |                   |
              +------------+-----------+-------------------+
              |     1      |  Logical  |         ||        |
              |            |     OR    |                   |
              +------------+-----------+-------------------+

This is pretty ugly, so I'm going to revert to one operator per line until someone can suggest better markup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Huh! The double quotes are there in the text version even when there is only one operator per line. Given I can't remove the ugliness, switching back to the multiple operators per line version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cabo Do you know a way of avoiding these extraneous double quotes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The extra quotes are added by xml2rfc, but I don't know why or how to avoid this. I posted to the xml2rfc mailing list about this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So it seems the extra quotes are caused by a bug which will be fixed in the upcoming release of xml2rfc. I propose to leave them be for now and pick up the new release of xml2rfc in due course.

Copy link
Collaborator

@gregsdennis gregsdennis left a comment

Choose a reason for hiding this comment

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

This looks good. Just some curiosity questions from me.

@glyn glyn force-pushed the predicate-improvements branch 6 times, most recently from 9e867b6 to 7b23a0a Compare August 7, 2021 01:30
@glyn glyn changed the base branch from pred to main August 7, 2021 01:32
@glyn glyn marked this pull request as ready for review August 7, 2021 01:33
* Generalise comparisons
* Make regex operator conform to Perl etc.
* Add operator precedence table

Note: there are some extraneous double quotes in the text rendering of
the operator precedence table. This is a known issue which should be
fixed in the next release of xml2rfc:
https://trac.ietf.org/trac/xml2rfc/ticket/600

Fixes #112
Fixes #113
Fixes #114
@glyn glyn force-pushed the predicate-improvements branch from 7b23a0a to 48af814 Compare August 10, 2021 09:11
@glyn glyn merged commit e143d98 into main Aug 10, 2021
@glyn glyn deleted the predicate-improvements branch August 10, 2021 09:13
@goessner
Copy link
Collaborator

goessner commented Aug 10, 2021 via email

@glyn
Copy link
Collaborator Author

glyn commented Aug 10, 2021

Thanks @goessner. I raised #116 to remove that remark.

@cabo cabo mentioned this pull request Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

regex comparison operator should be =~ Operator precedence table Generalise filter comparisons
3 participants