Skip to content

Conversation

@KyleJamesWalker
Copy link
Contributor

@KyleJamesWalker KyleJamesWalker commented Dec 8, 2022

Description

Add the contains operator.

Note: This hasn't been tested yet, I wanted to see about getting some feedback before proceeding with this PR.

Issues Resolved

#61

Check List

  • All tests pass.
  • New functionality includes testing.
  • New functionality has been documented in the README if applicable

kuldeepluvani
kuldeepluvani previously approved these changes Dec 8, 2022
Copy link

@kuldeepluvani kuldeepluvani left a comment

Choose a reason for hiding this comment

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

💯

@dmlb2000
Copy link
Member

dmlb2000 commented Dec 9, 2022

Can you give some examples in other places where ~= represents the in operation? Most of the places I'm aware of ~= is more of a regular expression match. Not what the in operator does in Python.

Also looks like you need some testing to cover the new code you created.

Thoughts?

@dmlb2000 dmlb2000 self-requested a review December 9, 2022 00:33
@KyleJamesWalker
Copy link
Contributor Author

KyleJamesWalker commented Dec 9, 2022

I'm currently trying to create some unit tests, it took me a little bit to get the antlr4 code generated, and now seeing what I might of missed to get past the:

token recognition error at: '~'

And let me look more into the the ~= vs contains, there's a good chance you're right, I was mainly just trying to figure out how to add to the code, before finalizing (hope it's not too distracting for you), thanks for the library!

I just checked and yes the ~ is for regex, I'll flip to:

contains | Checks if a string contains the specified substring (case-sensitive), or an array contains the specified element.

and maybe see if I can easily add regex or not.

@KyleJamesWalker KyleJamesWalker changed the title Add the ~= operator Support contains operator Dec 9, 2022
@KyleJamesWalker
Copy link
Contributor Author

@dmlb2000 I think this should cover the cases for contains, let me know if you'd like more tests?

Also it would be pretty easy to add the ~= to do a regex match on the value, would you like me to add that also? For my use case I really only needed a substring, but I can see where a full regex would be useful.

@KyleJamesWalker
Copy link
Contributor Author

@dmlb2000 I've also started adding the ~= but I'll have to understand the antlr4 syntax a bit more to allow escape characters, and possibly passing flags along with it.

I'll create a PR against this repo later but if you want to look at the possible changes I have a working branch on my fork if you have any advice: KyleJamesWalker#1

@dmlb2000
Copy link
Member

dmlb2000 commented Dec 14, 2022

@KyleJamesWalker I agree it will be a challenge to get the regex to pass through antlr to apply the standard regex library. However, if you feel you have the ability/time to do that I can review and help a little. Feel free to submit a new bug with the regex addition and make a merge request.

@KyleJamesWalker
Copy link
Contributor Author

@dmlb2000 How do you feel about this PR. Once this is merged, I can try and work on some more ideas on how to pass the flag to the regex matching. I at least have a small start so far with the 2nd PR.

Copy link
Member

@dmlb2000 dmlb2000 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 the contribution. Looks good to me!

@dmlb2000 dmlb2000 merged commit f7fff8e into pacifica:master Dec 14, 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.

3 participants