Skip to content

Add support for repeated sequences#60

Merged
Mikaayenson merged 19 commits intoendgameinc:masterfrom
Mikaayenson:master
Mar 16, 2022
Merged

Add support for repeated sequences#60
Mikaayenson merged 19 commits intoendgameinc:masterfrom
Mikaayenson:master

Conversation

@Mikaayenson
Copy link
Copy Markdown
Contributor

Resolves #57

Details

Updated the grammar and added sample test case to support repeated sequences.

@rw-access
Copy link
Copy Markdown
Contributor

Hi @Mikaayenson! It's exciting to see you dive right in to this issue. I think there are some more pieces that this is missing before it's ready to make sure all the edge cases are covered:

  • should only work with the Elasticsearch syntax on. otherwise, it should remain a syntax error (in Endgame mode)
  • it accidentally added new syntax sequence with runs=2 [ A ] [ B ]
  • this is now valid syntax: sequence [ A ] with runs=2 in Elasticsearch mode, but is not for Endgame mode
  • I think the AST should be expanded when the new syntax encountered: sequence [ A ] with runs=10 into sequence [ A ] [ A ] [ A ] [ A ] [ A ] [ A ] [ A ] [ A ] [ A ] [ A ]
  • more tests to confirm the behavior is matching what's expected

If you want to see an example of how those flags are handled, check out #59 or some of the other PRs that add Elasticsearch syntax handling behind the flag. I'm happy to sync up and go over anything in the code base if you need. Feel free to DM me (on ela.st/slack or zoom).

@Mikaayenson Mikaayenson changed the title Add support for repeated sequences WIP: Add support for repeated sequences Mar 14, 2022
@Mikaayenson Mikaayenson marked this pull request as draft March 14, 2022 14:35
@Mikaayenson Mikaayenson self-assigned this Mar 14, 2022
@Mikaayenson Mikaayenson changed the title WIP: Add support for repeated sequences Add support for repeated sequences Mar 14, 2022
@Mikaayenson Mikaayenson marked this pull request as ready for review March 14, 2022 20:43
Copy link
Copy Markdown
Contributor

@rw-access rw-access left a comment

Choose a reason for hiding this comment

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

wow, nice turnaround! that was fast. 🏃‍♂️
just a few nits and I think a couple of tests needed, looks really good so far

@Mikaayenson
Copy link
Copy Markdown
Contributor Author

@rw-access Thanks for the code walkthrough and feedback! I think it should be good to go now. 🤞

@Mikaayenson Mikaayenson requested a review from rw-access March 15, 2022 13:58
rw-access
rw-access previously approved these changes Mar 15, 2022
Copy link
Copy Markdown
Contributor

@rw-access rw-access left a comment

Choose a reason for hiding this comment

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

LGTM, just a few small tweaks

Copy link
Copy Markdown
Contributor

@brokensound77 brokensound77 left a comment

Choose a reason for hiding this comment

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

Nice job! I left a few comments

@rw-access rw-access dismissed their stale review March 15, 2022 20:32

Pending resolution to @brokensound77's comments

Copy link
Copy Markdown
Contributor

@brokensound77 brokensound77 left a comment

Choose a reason for hiding this comment

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

two small changes, then LGTM 🎉

@Mikaayenson
Copy link
Copy Markdown
Contributor Author

Alright! Thanks @brokensound77. I think its ready to go, but ill wait until tomorrow just in case there's anything else.

@Mikaayenson Mikaayenson merged commit 535bcdf into endgameinc:master Mar 16, 2022
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.

Add support for repeated sequences under elasticsearch_syntax

3 participants