Skip to content

Conversation

@codebrain
Copy link
Contributor

This commit adds support for the ip_range datatype.
The existing IpRange type used for IpRangeAggregation renamed to
IpRangeAggregationRange to allow for the introduction of an IpRange type
whose name aligns with other range datatypes.

This commit adds support for the ip_range datatype.
The existing IpRange type used for IpRangeAggregation renamed to
IpRangeAggregationRange to allow for the introduction of an IpRange type
whose name aligns with other range datatypes.
@codebrain codebrain requested a review from russcam April 26, 2018 08:15
Copy link
Contributor

@russcam russcam left a comment

Choose a reason for hiding this comment

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

LGTM - thoughts on naming IpAddressRange all over for consistency?

public IProperty LongRange(Func<LongRangePropertyDescriptor<T>, ILongRangeProperty> selector) =>
selector?.Invoke(new LongRangePropertyDescriptor<T>());

public IProperty IpRange(Func<IpAddressRangePropertyDescriptor<T>, IIpAddressRangeProperty> selector) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

IpAddressRange(...)? not sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was going to keep that unchanged...

Copy link
Member

Choose a reason for hiding this comment

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

+1 to IpRange since that's what it's called on the server

Copy link
Contributor

@russcam russcam left a comment

Choose a reason for hiding this comment

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

LGTM

@codebrain codebrain merged commit c729779 into 6.x Apr 27, 2018
@codebrain codebrain deleted the feature/ip-range-6.x branch April 27, 2018 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants