Issue 460 - datestartswith relational operator behavior on number expression #589
Conversation
- improve datestartswith operator handling of types - fix issue where equal was incorrectly used on default operators
- revert syntax tree for test failure
| } | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Without the change to the single column syntax tree
https://circleci.com/gh/plotly/dash-table/13136, 04bbe1d
and passing again afterwards
https://circleci.com/gh/plotly/dash-table/13143, 8087b40
| @@ -74,8 +74,8 @@ describe('Dash Table Queries', () => { | |||
|
|
|||
| describe('contains', () => { | |||
| processCases(c.syntaxer, [ | |||
| { name: 'cannot compare "11" to 1', query: `${c.hideOperand ? '' : '{a} '}contains 1`, target: { a: '11' }, valid: true, evaluate: true }, | |||
| { name: 'cannot compare 11 to 1', query: `${c.hideOperand ? '' : '{a} '}contains 1`, target: { a: 11 }, valid: true, evaluate: false }, | |||
| { name: 'compares "11" to 1', query: `${c.hideOperand ? '' : '{a} '}contains 1`, target: { a: '11' }, valid: true, evaluate: true }, | |||
| { name: 'cannot compare "11" to 1', query: `${c.hideOperand ? '' : '{a} '}contains 1`, target: { a: '11' }, valid: true, evaluate: true }, | ||
| { name: 'cannot compare 11 to 1', query: `${c.hideOperand ? '' : '{a} '}contains 1`, target: { a: 11 }, valid: true, evaluate: false }, | ||
| { name: 'compares "11" to 1', query: `${c.hideOperand ? '' : '{a} '}contains 1`, target: { a: '11' }, valid: true, evaluate: true }, | ||
| { name: 'compares 11 to 1', query: `${c.hideOperand ? '' : '{a} '}contains 1`, target: { a: 11 }, valid: true, evaluate: true }, |
There was a problem hiding this comment.
contains now does not distinguish between string 11 and number 11 in all (string|number, string|number) operand/expression combinations
| @@ -84,8 +84,8 @@ describe('Dash Table Queries', () => { | |||
| { name: 'compares "abc" to "b"', query: `${c.hideOperand ? '' : '{a} '}contains "b"`, target: { a: 'abc' }, valid: true, evaluate: true }, | |||
| { name: 'compares "abc" to " b"', query: `${c.hideOperand ? '' : '{a} '}contains " b"`, target: { a: 'abc' }, valid: true, evaluate: false }, | |||
| { name: 'compares "abc" to "b "', query: `${c.hideOperand ? '' : '{a} '}contains "b "`, target: { a: 'abc' }, valid: true, evaluate: false }, | |||
| { name: 'compares "abc" to " b"', query: `${c.hideOperand ? '' : '{a} '}contains " b"`, target: { a: 'a bc' }, valid: true, evaluate: true }, | |||
| { name: 'compares "abc" to "b "', query: `${c.hideOperand ? '' : '{a} '}contains "b "`, target: { a: 'ab c' }, valid: true, evaluate: true } | |||
| { name: 'compares "a bc" to " b"', query: `${c.hideOperand ? '' : '{a} '}contains " b"`, target: { a: 'a bc' }, valid: true, evaluate: true }, | |||
| @@ -96,7 +96,7 @@ describe('Dash Table Queries', () => { | |||
| { name: '0yyy in "0yyy"', query: `${c.hideOperand ? '' : '{a} '}datestartswith "0987"`, target: { a: '0987' }, valid: true, evaluate: true }, | |||
| { name: 'yyyy in "yyyy"', query: `${c.hideOperand ? '' : '{a} '}datestartswith "2006"`, target: { a: '2005' }, valid: true, evaluate: false }, | |||
| { name: 'yyyy in "yyyy"', query: `${c.hideOperand ? '' : '{a} '}datestartswith "2005"`, target: { a: '2005' }, valid: true, evaluate: true }, | |||
| { name: 'yyyy in yyyy', query: `${c.hideOperand ? '' : '{a} '}datestartswith 2005`, target: { a: '2005' }, valid: true, evaluate: false }, | |||
| { name: 'yyyy in yyyy', query: `${c.hideOperand ? '' : '{a} '}datestartswith 2005`, target: { a: '2005' }, valid: true, evaluate: true }, | |||
There was a problem hiding this comment.
the relational operator will now cast number to string for datestartswith
| op = typeof op === 'number' ? op.toString() : op; | ||
| exp = typeof exp === 'number' ? exp.toString() : exp; | ||
|
|
||
| return op.toString().indexOf(exp.toString()) !== -1; |
There was a problem hiding this comment.
isn't this double .toString?
But also: should we be trying to use the column formatting here? What if you have prices formatted with 2-digit decimals and you want to match all even-dollar values, with contains '.00'?
There was a problem hiding this comment.
Getting access to formatting from here would be difficult.
I think you're right, I got carried away by the number case for dates. Deviating too much from what was agreed upon when we last went through filtering. Will revert the contains changes and focus on (1) datestartswith and (2) the lexeme bug
There was a problem hiding this comment.
OK - certainly understand that this would be tough, and it's a weird edge case so I'm happy to ignore for now. Let's make an issue for it though, if it doesn't already exist. If a user is filtering as a string, they would expect the string used in the filter to be the one they see.
Fixes #460