-
Notifications
You must be signed in to change notification settings - Fork 443
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
Adjust reverse and reverseInternal visibility and type narrowing #582
Adjust reverse and reverseInternal visibility and type narrowing #582
Conversation
This change addresses an unintended consequence where the visibility of these methods as implemented by geometry subclasses was inconsistent. Signed-off-by: Jody Garnett <jody.garnett@gmail.com>
@@ -132,13 +132,12 @@ protected LinearRing copyInternal() { | |||
return new LinearRing(points.copy(), factory); | |||
} | |||
|
|||
/** @deprecated */ | |||
public Geometry reverse() | |||
public LinearRing reverse() |
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.
This is no longer deprecated, a change that was producing warnings in client code.
*/ | ||
public Geometry reverse() { | ||
return super.reverse(); | ||
public MultiLineString reverse() { |
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.
This is no longer deprecated, which was producing warnings in client code.
Discussion with @jnh5y asks about the use of Happy to add |
Is the type-narrowing needed? Or just the removal of |
Is it even necessary to have |
The one advantage is the type narrowing, in case where the geometry type is known. |
I decided to be explicit and check each case, the removal and deprecated and the change of method visibility are the priority. |
Alright, finally had a second to look into this more. Jody's type narrowing is the same as how copyInternal is currently handled on master. Given that precedent, I think the type narrowing is justified. |
} | ||
|
||
public Geometry reverseInternal() { | ||
public LinearRing reverseInternal() { |
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 believe this should be protected; otherwise this will be the only reverseInternal which has public scope!
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.
Thank you, good catch
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.
sorry I missed applying this feedback :(
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.
Actually, I think you switched reverse to protected instead of this method.
Signed-off-by: Jody Garnett <jody.garnett@gmail.com>
This change addresses an unintended consequence where the visibility of these methods as implemented by geometry subclasses was consistent.
Signed-off-by: Jody Garnett jody.garnett@gmail.com