-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Deprecate validate_* and normalize_*
#10248
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given how we've done other deprecations and removals, I think we need to maintain backcompat for at least reading and ignoring these settings in 2.0? Also, I think it is better to add move the version check to the else if and invert, then have a comment like // ignore legacy setting inside the block. For newer indexes, the setting would fall through and show up as an unknown setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that idea. I'm a big fan of avoiding exceptions if at all possible but wasn't sure how we've handled bwc in other cases. That will let us remove the 'breaking' label too? Committing a change shortly for review. Will also need to add more thorough testing before merging. I just needed this valuable feedback.
|
@nknize I left a couple comments. @clintongormley Maybe you have some thoughts on whether the breaking change should happen for 1.6 or 2.0? |
No need to carry 3 validate and 3 normalize options in GeoPointFieldMapping. This is just causing confusion so this change deprecates the validate_lat, validate_lon, normalize_lat, and normalize_lon options and simply uses validate and normalize, respectively. Geo filters have also been updated to include a validate and normalize option to provide consistency with the mapping defaults. Unit tests and documentation have been updated. closes elastic#10170
…e bwc functionality)
6ff3587 to
59f226b
Compare
|
@clintongormley I think deprecate in 1.6.0 and remove in 2.0.0, what do you think? |
|
Wondering if we should make the whole change (including removing |
|
@nknize any movement here? |
validate_* and normalize_*
|
@clintongormley looks like my issue notification was eaten by build failures. I'll deprecate in 1.6.1 and completely remove in 2.0. This one has hung around long enough. |
|
@nknize perhaps do something similar to the suggestion here: #11161 (comment) |
|
Bumping the version up to 1.7.1 for the today's release. |
|
closing this PR in favor of #12300 |
No need to carry 3 validate and 3 normalize options in GeoPointFieldMapping. This is just causing confusion. This change deprecates the validate_lat, validate_lon, normalize_lat, and normalize_lon options on the
geo_pointfield mapping and simplifies the options by using justvalidateandnormalize, respectively. Geo filters have been updated to include avalidateandnormalizeoption to provide consistency with the field mapping defaults. Unit tests and documentation have also been updated.closes #10170