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.

Sign up for GitHub

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

Jump to bottom

Allow a Group's children to be null #32

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Allow a Group's children to be null #32

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5dcd428
Allow a Group's children to be null
aslakhellesoy Oct 12, 2021
8231f8f
Merge remote-tracking branch 'origin/main' into null-children
aslakhellesoy Oct 14, 2021
d3a703b
Merge main
aslakhellesoy Oct 15, 2021
6edadd5
Update node versions
aslakhellesoy Oct 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions javascript/src/Argument.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ export default class Argument {

if (argGroups.length !== parameterTypes.length) {
throw new CucumberExpressionError(
`Group has ${argGroups.length} capture groups (${argGroups.map(
(g) => g.value
`Group has ${argGroups.length} capture groups (${argGroups.map((g) =>
g ? g.value : null
aurelien-reeves marked this conversation as resolved.
Show resolved Hide resolved
)}), but there were ${parameterTypes.length} parameter types (${parameterTypes.map(
(p) => p.name
)})`
Expand All @@ -22,7 +22,10 @@ export default class Argument {
return parameterTypes.map((parameterType, i) => new Argument(argGroups[i], parameterType))
}

constructor(public readonly group: Group, public readonly parameterType: ParameterType<unknown>) {
constructor(
public readonly group: Group | null,
public readonly parameterType: ParameterType<unknown>
) {
this.group = group
this.parameterType = parameterType
}
Expand All @@ -33,7 +36,7 @@ export default class Argument {
* @param thisObj the object in which the transformer function is applied.
*/
public getValue<T>(thisObj: unknown): T | null {
const groupValues = this.group ? this.group.values : null
const groupValues = this.group ? this.group.values : []
return this.parameterType.transform(thisObj, groupValues)
}

Expand Down
10 changes: 5 additions & 5 deletions javascript/src/Group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
export default class Group {
constructor(
public readonly value: string,
public readonly start: number | undefined,
public readonly end: number | undefined,
public readonly children: readonly Group[]
public readonly start: number,
public readonly end: number,
public readonly children: readonly (Group | null)[]
Copy link
Contributor

Choose a reason for hiding this comment

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

How should I read this? A list of nulls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mpkorstanje a list whose elements may be either null or aGroup object.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use null values in lists. That will be rather unexpected. If you do need something to represent the non capturing groups consider using the https://en.wikipedia.org/wiki/Null_object_pattern

) {}

get values(): string[] | null {
return (this.children.length === 0 ? [this] : this.children).map((g) => g.value)
get values(): (string | null)[] {
return (this.children.length === 0 ? [this] : this.children).map((g) => (g ? g.value : null))
}
}
7 changes: 4 additions & 3 deletions javascript/src/GroupBuilder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,14 @@ export default class GroupBuilder {
this.groupBuilders.push(groupBuilder)
}

public build(match: RegExpExecArray, nextGroupIndex: () => number): Group {
public build(match: RegExpExecArray, nextGroupIndex: () => number): Group | null {
const groupIndex = nextGroupIndex()
const children = this.groupBuilders.map((gb) => gb.build(match, nextGroupIndex))
const value = match[groupIndex]
if (value === undefined) return null
const index = match.indices[groupIndex]
const start = index ? index[0] : undefined
const end = index ? index[1] : undefined
const start = index[0]
const end = index[1]
return new Group(value, start, end, children)
}

Expand Down
2 changes: 1 addition & 1 deletion javascript/src/ParameterType.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export default class ParameterType<T> {
this.transformFn = transform
}

public transform(thisObj: unknown, groupValues: string[] | null) {
public transform(thisObj: unknown, groupValues: (string | null)[]) {
return this.transformFn.apply(thisObj, groupValues)
}
}
Expand Down
6 changes: 3 additions & 3 deletions javascript/test/CucumberExpressionTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ describe('CucumberExpression', () => {
assert.strictEqual(new CucumberExpression(expr, new ParameterTypeRegistry()).source, expr)
})

it('unmatched optional groups have undefined values', () => {
it('unmatched optional groups have null values', () => {
const parameterTypeRegistry = new ParameterTypeRegistry()
parameterTypeRegistry.defineParameterType(
new ParameterType(
Expand All @@ -124,8 +124,8 @@ describe('CucumberExpression', () => {

const world = {}

assert.deepStrictEqual(expression.match(`TLA`)![0].getValue(world), ['TLA', undefined])
assert.deepStrictEqual(expression.match(`123`)![0].getValue(world), [undefined, '123'])
assert.deepStrictEqual(expression.match(`TLA`)![0].getValue(world), ['TLA', null])
assert.deepStrictEqual(expression.match(`123`)![0].getValue(world), [null, '123'])
})

// JavaScript-specific
Expand Down
2 changes: 0 additions & 2 deletions javascript/test/RegularExpressionTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,11 @@ describe('RegularExpression', () => {
it('documents match arguments', () => {
const parameterRegistry = new ParameterTypeRegistry()

/// [capture-match-arguments]
const expr = /I have (\d+) cukes? in my (\w+) now/
const expression = new RegularExpression(expr, parameterRegistry)
const args = expression.match('I have 7 cukes in my belly now')!
assert.strictEqual(7, args[0].getValue(null))
assert.strictEqual('belly', args[1].getValue(null))
/// [capture-match-arguments]
})

it('does no transform by default', () => {
Expand Down
38 changes: 19 additions & 19 deletions javascript/test/TreeRegexpTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ describe('TreeRegexp', () => {
const tr = new TreeRegexp(/(a(?:b)?)(c)/)
const group = tr.match('ac')!
assert.strictEqual(group.value, 'ac')
assert.strictEqual(group.children[0].value, 'a')
assert.deepStrictEqual(group.children[0].children, [])
assert.strictEqual(group.children[1].value, 'c')
assert.strictEqual(group.children[0]!.value, 'a')
assert.deepStrictEqual(group.children[0]!.children, [])
assert.strictEqual(group.children[1]!.value, 'c')
aurelien-reeves marked this conversation as resolved.
Show resolved Hide resolved
})

it('ignores `?:` as a non-capturing group', () => {
Expand All @@ -39,37 +39,37 @@ describe('TreeRegexp', () => {
const group = tr.match('abc')!
assert.strictEqual(group.value, 'abc')
assert.strictEqual(group.children.length, 1)
assert.strictEqual(group.children[0].value, 'bc')
assert.strictEqual(group.children[0]!.value, 'bc')
})

it('ignores `?<=` as a non-capturing group', () => {
const tr = new TreeRegexp(/a(.+)(?<=c)$/)
const group = tr.match('abc')!
assert.strictEqual(group.value, 'abc')
assert.strictEqual(group.children.length, 1)
assert.strictEqual(group.children[0].value, 'bc')
assert.strictEqual(group.children[0]!.value, 'bc')
})

it('ignores `?<!` as a non-capturing group', () => {
const tr = new TreeRegexp(/a(.+?)(?<!b)$/)
const group = tr.match('abc')!
assert.strictEqual(group.value, 'abc')
assert.strictEqual(group.children.length, 1)
assert.strictEqual(group.children[0].value, 'bc')
assert.strictEqual(group.children[0]!.value, 'bc')
})

it('matches named capturing group', () => {
const tr = new TreeRegexp(/a(?<name>b)c/)
const group = tr.match('abc')!
assert.strictEqual(group.value, 'abc')
assert.strictEqual(group.children.length, 1)
assert.strictEqual(group.children[0].value, 'b')
assert.strictEqual(group.children[0]!.value, 'b')
})

it('matches optional group', () => {
const tr = new TreeRegexp(/^Something( with an optional argument)?/)
const group = tr.match('Something')!
assert.strictEqual(group.children[0].value, undefined)
assert.strictEqual(group.children[0], null)
})

it('matches nested groups', () => {
Expand All @@ -78,15 +78,15 @@ describe('TreeRegexp', () => {
)
const group = tr.match('A 5 thick line from 10,20,30 to 40,50,60')!

assert.strictEqual(group.children[0].value, '5')
assert.strictEqual(group.children[1].value, '10,20,30')
assert.strictEqual(group.children[1].children[0].value, '10')
assert.strictEqual(group.children[1].children[1].value, '20')
assert.strictEqual(group.children[1].children[2].value, '30')
assert.strictEqual(group.children[2].value, '40,50,60')
assert.strictEqual(group.children[2].children[0].value, '40')
assert.strictEqual(group.children[2].children[1].value, '50')
assert.strictEqual(group.children[2].children[2].value, '60')
assert.strictEqual(group.children[0]!.value, '5')
assert.strictEqual(group.children[1]!.value, '10,20,30')
assert.strictEqual(group.children[1]!.children[0]!.value, '10')
assert.strictEqual(group.children[1]!.children[1]!.value, '20')
assert.strictEqual(group.children[1]!.children[2]!.value, '30')
assert.strictEqual(group.children[2]!.value, '40,50,60')
assert.strictEqual(group.children[2]!.children[0]!.value, '40')
assert.strictEqual(group.children[2]!.children[1]!.value, '50')
assert.strictEqual(group.children[2]!.children[2]!.value, '60')
})

it('detects multiple non capturing groups', () => {
Expand Down Expand Up @@ -117,7 +117,7 @@ describe('TreeRegexp', () => {
const tr = new TreeRegexp('the stdout(?: from "(.*?)")?')
const group = tr.match('the stdout')!
assert.strictEqual(group.value, 'the stdout')
assert.strictEqual(group.children[0].value, undefined)
assert.strictEqual(group.children[0], null)
assert.strictEqual(group.children.length, 1)
})

Expand Down Expand Up @@ -146,6 +146,6 @@ describe('TreeRegexp', () => {
const group = tr.match('drawings: ONE(TWO)')!
assert.strictEqual(group.value, 'drawings: ONE(TWO)')
assert.strictEqual(group.children.length, 1)
assert.strictEqual(group.children[0].value, 'ONE(TWO)')
assert.strictEqual(group.children[0]!.value, 'ONE(TWO)')
})
})