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

make 'includes' and 'does not include' work as substring tests #44

Closed
cgrayson opened this issue Nov 16, 2015 · 6 comments
Closed

make 'includes' and 'does not include' work as substring tests #44

cgrayson opened this issue Nov 16, 2015 · 6 comments

Comments

@cgrayson
Copy link
Contributor

it looks like includes and does not include could never behave differently than is equal to and is not equal to. As in this test.

Discussion.

@alexj
Copy link

alexj commented Nov 16, 2015

We'd like the ability to check whether a string contains another string, so we should ensure that includes and does not include work in this manner instead of removing the operators from the string type. Or am I missing something?

@cgrayson
Copy link
Contributor Author

I think that's a separate issue. Currently, at least, includes and does not include have specific meaning with regard to arrays. Though they seem similar, includes is explicitly written to not do pattern matching in that context, so I'd be worried about "overloading" the operator like that.

For example, ['apple', 'beetle', 'carpenter'] includes 'bee' would be false (but that array includes 'beetle' would be true).

Maybe it would be fine. Maybe there should be a contains or matches instead. Maybe Alex W. has it all figured out & half written. 😃

@alexj
Copy link

alexj commented Nov 16, 2015

That makes sense. contains would make more sense as a label too. Thanks Chris.

@alexkwolfe
Copy link
Contributor

I also think what you've said makes sense, Chris. 👍

@cgrayson cgrayson self-assigned this Sep 2, 2016
cgrayson added a commit that referenced this issue Sep 2, 2016
I don't think this will affect lead handling, if flows have rules
in place testing 'string' values with these operators. It will
cause validation warnings on those flows the next time they're
saved.
cgrayson referenced this issue Sep 2, 2016
Remove 'string' operators 'includes' & 'does not include'
@cgrayson cgrayson reopened this Sep 2, 2016
@cgrayson cgrayson changed the title remove 'includes', 'does not include' operators from 'string' make 'includes' and 'does not include' work as substring tests Sep 2, 2016
@cgrayson
Copy link
Contributor Author

cgrayson commented Sep 2, 2016

After further discussion… 😊 rather than remove these, Wolfe & I decided it would be better to make them operate as substring tests (i.e., as Jones mentioned in the first place), after all.

These still won't be full regex operators, though; that would still be a separate operator (say, matches).

For the record we also talked about making this string operator have a distinct name other than the array operator (includes), such as contains. There's potential user confusion, when working with a rule, if they're offered operators that don't match what they're expecting based on data type. In the end we think it will be less confusing to have includes mean similar things with strings & arrays, instead of having different, synonymous names.

@cgrayson cgrayson reopened this Sep 2, 2016
@cgrayson cgrayson removed their assignment Sep 2, 2016
alexkwolfe pushed a commit that referenced this issue Sep 12, 2016
I don't think this will affect lead handling, if flows have rules
in place testing 'string' values with these operators. It will
cause validation warnings on those flows the next time they're
saved.
@cgrayson
Copy link
Contributor Author

Looking at this now, I screwed up which issue should be open or closed. The valid string operators are listed here, but the way they work is defined in leadconduit-rules. So, as I should have done on Sep 2, I'm: closing this, and reopening rules #26.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants