Skip to content

Conversation

@codebrain
Copy link
Contributor

@codebrain codebrain commented Apr 26, 2018

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 06:03
@codebrain codebrain changed the title Backport of #3202 to 5.x Add support for ip_range datatype (Backport of #3202 to 5.x) Apr 26, 2018
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.

Left some comments

[JsonProperty(PropertyName = "ranges")]
IEnumerable<IIpRange> Ranges { get; set; }
[JsonProperty("ranges")]
IEnumerable<IIpRangeAggregationRange> Ranges { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert rename; breaks BWC

{
public Field Field { get; set; }
public IEnumerable<IIpRange> Ranges { get; set; }
public IEnumerable<IIpRangeAggregationRange> Ranges { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert rename; breaks BWC

Field IIpRangeAggregation.Field { get; set; }

IEnumerable<IIpRange> IIpRangeAggregation.Ranges { get; set; }
IEnumerable<IIpRangeAggregationRange> IIpRangeAggregation.Ranges { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert rename; breaks BWC


public IpRangeAggregationDescriptor<T> Ranges(params Func<IpRangeDescriptor, IIpRange>[] ranges) =>
Assign(a => a.Ranges = ranges?.Select(r => r(new IpRangeDescriptor())));
public IpRangeAggregationDescriptor<T> Ranges(params Func<IpRangeAggregationRangeDescriptor, IIpRangeAggregationRange>[] ranges) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert rename; breaks BWC

public IpRangeAggregationDescriptor<T> Ranges(params Func<IpRangeDescriptor, IIpRange>[] ranges) =>
Assign(a => a.Ranges = ranges?.Select(r => r(new IpRangeDescriptor())));
public IpRangeAggregationDescriptor<T> Ranges(params Func<IpRangeAggregationRangeDescriptor, IIpRangeAggregationRange>[] ranges) =>
Assign(a => a.Ranges = ranges?.Select(r => r(new IpRangeAggregationRangeDescriptor())));
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert rename; breaks BWC


namespace Nest
{
public class IpRangeAttribute : RangePropertyAttributeBase, IIpRangeProperty
Copy link
Contributor

Choose a reason for hiding this comment

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

IpAddressRangeAttribute, to align with IpAddressRange?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👎

namespace Nest
{
[JsonObject(MemberSerialization.OptIn)]
public interface IIpRangeProperty : IRangeProperty { }
Copy link
Contributor

Choose a reason for hiding this comment

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

IIpAddressRangeProperty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👎

[JsonObject(MemberSerialization.OptIn)]
public interface IIpRangeProperty : IRangeProperty { }

public class IpRangeProperty : RangePropertyBase, IIpRangeProperty
Copy link
Contributor

Choose a reason for hiding this comment

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

IpAddressRangeProperty ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👎

public IpRangeProperty() : base(RangeType.IpRange) { }
}

public class IpRangePropertyDescriptor<T>
Copy link
Contributor

Choose a reason for hiding this comment

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

IpAddressRangePropertyDescriptor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👎

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, just some small changes for naming consistency

using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
using Elasticsearch.Net;
Copy link
Contributor

Choose a reason for hiding this comment

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

needed?


namespace Tests.Mapping.Types.Core.Range.IpRange
{
public class IpRangeTest
Copy link
Contributor

Choose a reason for hiding this comment

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

IpAddressRangeTest for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👎

}

[SkipVersion("<5.5.0", "ip range type is a new 5.5.0 feature")]
public class IpRangeAttributeTests : AttributeTestsBase<IpRangeTest>
Copy link
Contributor

Choose a reason for hiding this comment

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

IpAddressRangeAttributeTests for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👎

namespace Tests.Mapping.Types.Core.Range.IpRange
{
[SkipVersion("<5.5.0", "ip range type is a new 5.5.0 feature")]
public class IpRangePropertyTests : PropertyTestsBase
Copy link
Contributor

Choose a reason for hiding this comment

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

IpAddressRangePropertyTests for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👎

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 37a34e0 into 5.x Apr 27, 2018
@codebrain codebrain deleted the feature/ip-range-5.x branch April 27, 2018 02:55
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.

3 participants