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
[GEO] 6x Deprecate ShapeBuilders and decouple geojson parse logic #27345
Conversation
@cbuescher This is the 6.x version of the #27212 PR. There are only 2 deprecations. |
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.
@nknize great, I left a few comments (mostly very minor nits, some might also be good to check on master), the deprecation looks good otherwise.
|
||
/** | ||
* Created by nknize on 9/22/17. | ||
*/ |
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.
Nit: only place I seen this so far, auto-created?
*/ | ||
@Deprecated |
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.
public static void register(List<Entry> namedWriteables)
is unused and can probably already be removed, should only be used internally.
* @param latitude latitude of the point | ||
* @return a new {@link PointBuilder} | ||
*/ | ||
public static PointBuilder newPoint(double longitude, double latitude) { |
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.
Where does this come from? Seems unused.
/** @deprecated this method will be removed in a future version; use ShapeParser.parse instead */ | ||
@Deprecated | ||
public static ShapeBuilder parse(XContentParser parser) throws IOException { | ||
DEPRECATION_LOGGER.deprecated("deprecated method [ShapeBuilder.parse] used; replaced by [ShapeParser.parse]"); |
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 don't think we need deprecation logging here, I think marking the java API should be enough.
* @param coordinate | ||
* Coordinate for the Node | ||
*/ | ||
protected CoordinateNode(Coordinate coordinate) { |
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.
nit: can be package private (maybe I didn't see that in the master PR)
* @param children | ||
* Children of the Node | ||
*/ | ||
protected CoordinateNode(List<CoordinateNode> children) { |
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.
nit: can be package private (maybe I didn't see that in the master PR)
return (coordinate == null && (children == null || children.isEmpty())); | ||
} | ||
|
||
public boolean isMultiPoint() { |
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.
seems unused
174ec45
to
64c30e0
Compare
8af2d00
to
a3fd30f
Compare
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.
@nknize thanks for adding those changes, LGTM
a3fd30f
to
162453c
Compare
Like 7.0 this commit refactors the geo_shape parsing logic into its own package separate from the Shape builders and deprecates ShapeBuilders helper classes and methods. It also decouples the GeoShapeType into a standalone enumerator that is responsible for validating the parsed data and providing the appropriate builder. This future-proofs the code making it easier to maintain and not only add new shape types but support for new geo formats (e.g., WKT).