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

Delete forecast API #3591

Merged
merged 11 commits into from
Mar 19, 2019
Merged

Delete forecast API #3591

merged 11 commits into from
Mar 19, 2019

Conversation

codebrain
Copy link
Contributor

No description provided.

@codebrain
Copy link
Contributor Author

Also raised an issue here: elastic/elasticsearch#39673

@codebrain codebrain mentioned this pull request Mar 5, 2019
Closed
48 tasks
@codebrain codebrain requested a review from Mpdreamz March 6, 2019 00:59
russcam
russcam reviewed Mar 6, 2019
View reviewed changes
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.

Had a question about forecast_id

...ion/ApiGenerator/RestSpecification/XPack/MachineLearning/xpack.ml.delete_forecast.patch.json Show resolved Hide resolved
russcam
russcam reviewed Mar 13, 2019
View reviewed changes
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.

I left some additional comments

src/Nest/CommonAbstractions/Infer/ForecastIds/ForecastIds.cs Outdated Show resolved Hide resolved
src/Nest/CommonAbstractions/Infer/ForecastIds/ForecastIds.cs Outdated Show resolved Hide resolved
src/Nest/CommonAbstractions/Infer/ForecastIds/ForecastIds.cs Show resolved Hide resolved
src/Nest/CommonAbstractions/Infer/ForecastIds/ForecastIds.cs Show resolved Hide resolved
src/Nest/CommonAbstractions/Infer/ForecastIds/ForecastIds.cs Outdated Show resolved Hide resolved
@russcam
Copy link
Contributor

russcam commented Mar 18, 2019 •
edited
Loading

I've updated the ForecastIds implementation to:

  • implement GetHashCode() based on the sorted contents
  • remove internal IReadOnlyList<string> property
  • All static property initializer
  • Remove the conditional operators in implicit operator conversions into the ctors

Added unit tests for Equality and GetHashCode

@codebrain codebrain merged commit d7e1260 into 6.6 Mar 19, 2019
@codebrain codebrain deleted the feature/ml-api-delete-forecast branch March 19, 2019 03:39
russcam pushed a commit that referenced this pull request Mar 20, 2019
@russcam
Delete forecast API (#3591)
7ad67ec 
Implement Delete Forecast API + integration tests
russcam pushed a commit that referenced this pull request Mar 21, 2019
@russcam
Delete forecast API (#3591)
e1a2a13 
Implement Delete Forecast API + integration tests
russcam pushed a commit that referenced this pull request Apr 3, 2019
@russcam
Delete forecast API (#3591)
6645893 
Implement Delete Forecast API + integration tests

(cherry picked from commit e1a2a13)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@russcam russcam russcam left review comments

@Mpdreamz Mpdreamz Awaiting requested review from Mpdreamz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@codebrain @russcam @Mpdreamz @Henr1k80