Allow a Group's children to be null

[aslakhellesoy]

Description

Change the type of Group#children from Group[] to (Group | null)[]

Motivation & context

Currently, it is possible for a Group object's value, start and end fields to be null / nil / undefined. This happens for optional capture groups.

A Group object where all fields are absent isn't very useful, and I find it more intuitive that the entire group object is null.

If there are no objections to this change I can update the other implementations.

Type of change

• Breaking change (will cause existing functionality to not
  work as expected)

Checklist:

• I have read the CONTRIBUTING document.
• My code follows the code style of this project.
• I have added tests to cover my changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

[mpkorstanje]

Wouldn't an empty list make more sense then null?

[mpkorstanje]

How should I read this? A list of nulls?

[aslakhellesoy]

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

[mpkorstanje]

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

[mpkorstanje]

I'm thinking that there may be some counting of groups going on. So replacing a non-capturing group with null doesn't make much sense and is bad programming (lists are usually free from nulls) So rather perhaps use (Group | NonCapturingGroup)[] where the NonCapturingGroup acts like the null object pattern ?

[akostadinov]

What will change from user point of view? What kind of steps would be affected and how?

[aslakhellesoy]

I'm thinking that there may be some counting of groups going on. So replacing a non-capturing group with null doesn't make much sense and is bad programming (lists are usually free from nulls) So rather perhaps use (Group | NonCapturingGroup)[] where the NonCapturingGroup acts like the null object pattern ?

That's an option, we can explore that.

In that case I'd rename the current Group to CapturingGroup.

A NonCapturingGroup does not have a value, start or end, so we have to either leave these properties as null (which would essentially be the same as the current behaviour which I dislike), or we only declare these properties on the CapturingGroup class.

We could make both CapturingGroup and NonCapturingGroup implement a Group interface that only has a children property, and possibly an isCapturing(): boolean method on it.

Would you prefer this @mpkorstanje?

[mpkorstanje]

I'd say that CapturingGroup with the properties and NonCapturingGroup withouth would be the right way to model this. Then for convenience a Group interface would be useful too.

But more importantly, do we need the non capturing groups for anything? If not, could we perhaps leave them out of the list all together?

[aslakhellesoy]

But more importantly, do we need the non capturing groups for anything? If not, could we perhaps leave them out of the list all together?

That would be nice.

I think we need the non capturing groups in order to provide null values when we call Argument#getValue() because ParameterType#transform() expects the same number of arguments as there are capture group.

I'll have to play around with it a bit.