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.

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

downgrade to java 7 #833

Closed
wants to merge 1 commit into from
Closed

downgrade to java 7 #833

wants to merge 1 commit into from

Conversation

osana
Copy link
Contributor

@osana osana commented Jun 7, 2018

closes #868
related to #832

java8 was introduced by #770 #749

@osana osana requested review from tobrun and LukasPaczos and removed request for tobrun and LukasPaczos June 7, 2018 16:22
@osana osana closed this Jun 7, 2018
@osana osana reopened this Aug 15, 2018
Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be able to rebase on master? I'm not seeing https://github.com/mapbox/mapbox-java/blob/master/services-geojson/src/main/java/com/mapbox/geojson/Geometry.java#L26 being adressed which shouldn't compile with 1.7

@@ -17,5 +17,5 @@
* @return the {@link Point}s which make up the coordinates defining the geometry
* @since 3.0.0
*/
T coordinates();
public abstract T coordinates();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the previous method was package private, can we retain that with this setup?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was part of an interface so when implemented it would have to be public

@@ -84,7 +86,7 @@ public void testToFromJson1() {
.distance(53.4)
//.weight(14.3)
.duration(14.3)
.steps(new ArrayList<>())
.steps(legSteps)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these changes seem unrelated but no biggy to keep them around

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason it was giving me compilation error under java 7.
Casting was not possible for some reason.

@@ -7,7 +7,7 @@
*
* @since 1.0.0
*/
public interface GeoJson {
public abstract class GeoJson {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

geojson itself is public and changing it from an interface to an abstract class would be a breaking change as users using it will need to change implements to extends + if they are already extending an object, they won't be able to make this change due to java only supporting single inheritance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tobrun Good point. I will create another PR then with the method just removed.

@osana
Copy link
Contributor Author

osana commented Aug 16, 2018

I did not have to remove
https://github.com/mapbox/mapbox-java/blob/master/services-geojson/src/main/java/com/mapbox/geojson/Geometry.java#L26
as it used to be a default method in the interface but now it is a method in an abstract class.

@osana osana closed this Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Java 8 features from geojson module
2 participants