Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Add spaces to regex for relational operators. #612

Merged
merged 19 commits into from
Oct 8, 2019

Conversation

shammamah-zz
Copy link
Contributor

Closes #563.

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-612 October 2, 2019 21:31 Inactive
Since gt is now interpreted as a string, it was removed from the invalid filters test.
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 4, 2019 15:24 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 4, 2019 15:25 Inactive
@shammamah-zz shammamah-zz marked this pull request as ready for review October 4, 2019 15:46
@alexcjohnson
Copy link
Collaborator

I think all the letter-based operators need this space - for example I can type containsb right now and it's interpreted as contains b

I think you were right to exclude the symbol operators - so <5 still works as < 5 - let's add a test though to lock that down.

@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 4, 2019 18:05 Inactive
@shammamah-zz shammamah-zz force-pushed the text-comparison-operators branch from 969a97b to f29e9bc Compare October 4, 2019 18:07
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 4, 2019 18:07 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 4, 2019 18:27 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 4, 2019 18:38 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 4, 2019 18:53 Inactive
Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 4, 2019 20:43 Inactive
}, LEXEME_BASE);

export const lessThan: IUnboundedLexeme = R.merge({
evaluate: relationalEvaluator(([op, exp]) => op < exp),
subType: RelationalOperator.LessThan,
regexp: /^(<|lt)/i
regexp: /^(<|lt\s)/i
}, LEXEME_BASE);

export const notEqual: IUnboundedLexeme = R.merge({
evaluate: relationalEvaluator(([op, exp]) => op !== exp),
subType: RelationalOperator.NotEqual,
regexp: /^(!=|ne)/i
Copy link
Contributor

Choose a reason for hiding this comment

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

ne should also have \s

@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 4, 2019 21:07 Inactive
@@ -67,19 +67,19 @@ export const equal: IUnboundedLexeme = R.merge({
op === exp
),
subType: RelationalOperator.Equal,
regexp: /^(=|eq)/i
regexp: /^(=|eq\s)/i
Copy link
Contributor

Choose a reason for hiding this comment

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

And the other ones :)

@Marc-Andre-Rivet
Copy link
Contributor

Needs changelog entry

@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 7, 2019 14:34 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 7, 2019 14:37 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 7, 2019 15:33 Inactive
Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 7, 2019 15:52 Inactive
@Marc-Andre-Rivet
Copy link
Contributor

@shammamah The percy diffs are interesting.. looks like there’s an extra space in the filter string now. Possibly a side effect of the new selector? There’s code that destructures/restructures that request from the individual column fragments. I suspect the new space is added somewhere in those steps.

@Marc-Andre-Rivet
Copy link
Contributor

There’s a “transform” on the lexemes that might need to be updated on the relational operators to take the (double?) space into account.

@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 7, 2019 18:12 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 7, 2019 19:36 Inactive
This reverts commit aae4907.
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 7, 2019 20:32 Inactive
@shammamah-zz shammamah-zz force-pushed the text-comparison-operators branch from 475d999 to 355be90 Compare October 7, 2019 20:37
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 7, 2019 20:37 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 7, 2019 22:03 Inactive
@@ -57,7 +57,8 @@ export const contains: IUnboundedLexeme = R.merge({
op.toString().indexOf(exp.toString()) !== -1
),
subType: RelationalOperator.Contains,
regexp: /^(contains)/i
regexp: /^((contains)(?=\s|$))/i,
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet Oct 8, 2019

Choose a reason for hiding this comment

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

I had some browser support concerns but turns out lookahead is widely supported and not an issue. Lookbehind is not https://caniuse.com/#feat=js-regexp-lookbehind

@shammamah-zz shammamah-zz temporarily deployed to dash-table-review-pr-612 October 8, 2019 03:28 Inactive
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet 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. Good job making this change super concise!

@shammamah-zz shammamah-zz merged commit c1d617b into dev Oct 8, 2019
@shammamah-zz shammamah-zz deleted the text-comparison-operators branch October 8, 2019 13:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text Comparison Operators
4 participants