extended_bounds type on DateHistogram

[FabienLavocat]

NEST/Elasticsearch.Net version: NEST 5.5.0
Elasticsearch version: 5.5.1
Description of the problem including expected versus actual behavior: The class DateHistogramAggregation has a property ExtendedBounds with the type ExtendedBounds<DateTime>
I think this type should be more generic. Having the type ExtendedBounds<DateMath> would make it easier to use values like "now-1d".

In a JSON message, I can send the following:

"over_time" : {
  "date_histogram" : {
  "field" : "date",
  "interval" : "hour",
  "extended_bounds": {
   "min": "now-12h",
   "max": "now"
  }
 }
}

PS: I can send a PR if needed.

[russcam]

Hey @FabienLavocat, thanks for reporting this.

I agree that supporting ExtendedBounds<DateMath> would be much better and a PR against master for this would be awesome! 👍

It gets a little trickier with 2.x and 5.x branches however; technically changing from ExtendedBounds<DateTime> to ExtendedBounds<DateMath> would be a binary breaking change, although there is an implicit conversion from DateTime to DateMath .

For binary backwards compatibility, a PR against 5.x and 2.x would need to deprecate the ExtendedBounds<DateTime> property, add a ExtendedBounds<DateMath> with a different name and assign to this when the deprecated property is assigned to, similar to how "rewrite" was handled. If you'd like to submit a PR for this too, it would be greatly appreciated, otherwise could be easily done from a PR against master.

[FabienLavocat]

I've made the change in the master branch. I don't think it would be an issue in 5.x since there is the implicit conversion from DateTime to DateMath

[russcam]

Thanks for the PR, @FabienLavocat 🎉

The breaking issue is not so much with implicit conversion from DateTime -> DateMath, but more so that the property on the request is ExtendedBounds<DateTime> and changing to ExtendedBounds<DateMath> would be binary breaking change with no implicit conversion.

[FabienLavocat]

Oh yes, it makes complete sense. I'll see what's possible for 5.x

[russcam]

Closing this as PRs merged for the next releases. Thanks for opening @FabienLavocat 👍