-
-
Notifications
You must be signed in to change notification settings - Fork 517
feat: Add array length function, closes #3308 #3317
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
base: main
Are you sure you want to change the base?
Changes from all commits
355661d
b87f922
7eecaef
b8c20ba
c4c333b
f00633c
d6b668f
e8e43cf
4babc0b
89d945a
c1d2435
c5425bd
983f997
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,38 @@ | |
import { ExpressionFunctions } from '../lib/Expression/ExpressionFunctions.js' | ||
|
||
describe('functions', () => { | ||
describe('general', () => { | ||
it('length', () => { | ||
expect(ExpressionFunctions.length()).toBe(0) | ||
expect(ExpressionFunctions.length('')).toBe(0) | ||
expect(ExpressionFunctions.length('a')).toBe(1) | ||
expect(ExpressionFunctions.length('abc')).toBe(3) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. expect(ExpressionFunctions.length('ä')).toBe(1) // codepoint U+00E4, one grapheme There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems this is pretty hard to do entirely properly, but do you think of these is "good enough"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually node.js has the Intl.segmenter() since version 16 but that is not working 100% with combining marks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I've got unicode-segmenter going now, but it doesn't return the right result for all cases for some reason (unless I've made a stupid error somewhere)? |
||
expect(ExpressionFunctions.length('ä')).toBe(1) // codepoint U+00E4, one grapheme | ||
expect(ExpressionFunctions.length('̈a')).toBe(1) // codepoints U+0308 U+0061, one grapheme | ||
Check failure on line 12 in shared-lib/test/expressions-functions.test.ts
|
||
expect(ExpressionFunctions.length('́̈a')).toBe(1) // codepoints U+0301 U+0308 U+0061, one grapheme | ||
expect(ExpressionFunctions.length(9)).toBe(1) | ||
expect(ExpressionFunctions.length(99)).toBe(2) | ||
expect(ExpressionFunctions.length(-123)).toBe(4) | ||
expect(ExpressionFunctions.length(3.14)).toBe(4) | ||
expect(ExpressionFunctions.length(BigInt(1024))).toBe(4) | ||
expect(ExpressionFunctions.length(BigInt(9007199254740991))).toBe(16) | ||
expect(ExpressionFunctions.length(new RegExp('ab+c', 'i'))).toBe(7) | ||
expect(ExpressionFunctions.length([])).toBe(0) | ||
expect(ExpressionFunctions.length([9])).toBe(1) | ||
expect(ExpressionFunctions.length([99])).toBe(1) | ||
expect(ExpressionFunctions.length(['abc'])).toBe(1) | ||
expect(ExpressionFunctions.length([9, 'a'])).toBe(2) | ||
expect(ExpressionFunctions.length(['a', 'c'])).toBe(2) | ||
peternewman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
expect(ExpressionFunctions.length(['ab', ''])).toBe(2) | ||
expect(ExpressionFunctions.length([1, , 3])).toBe(3) | ||
expect(ExpressionFunctions.length(['a', 'b', 'c'])).toBe(3) | ||
expect(ExpressionFunctions.length(['a', ['b', 'b'], 'c'])).toBe(3) | ||
expect(ExpressionFunctions.length({ a: 1 })).toBe(1) | ||
expect(ExpressionFunctions.length({ a: 1, b: { c: 5 } })).toBe(2) | ||
expect(ExpressionFunctions.length({ a: ['a', 'c'], b: { c: 5 } })).toBe(2) | ||
}) | ||
}) | ||
|
||
describe('number', () => { | ||
it('round', () => { | ||
expect(ExpressionFunctions.round(9.99)).toBe(10) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all uncatched types have a lenght property, e.g. BIGINT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added BigInt. What others are you aware (RegExp maybe) of/what do you think I should do in the default case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added RegExp too now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I think string should have its own case and then it is not part of the unknown rest any more. The unknown rest could maybe return NaN.
From the JS perspective there are also iterators and generators that are interesting for length() but currently we don't use them in variables, so if you want to implement them it would be a bonus for possible future use. Also we don't use regex data type currently and I'm not sure about the value of knowing the length of a regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I agree, a string case would be good and the fallback should always return NaN.
Another note is that
typeof new Regexp()
isobject
, so this will need to be above the object checkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I've added/sorted those others, useful or not and made it return NaN as the default otherwise.
I guess ideally we need yet another type of object if we wish to test the NaN case...