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

feat: Adding PolyUtil extensions to List<LatLng> #16

Merged
merged 3 commits into from
Mar 18, 2020
Merged

feat: Adding PolyUtil extensions to List<LatLng> #16

merged 3 commits into from
Mar 18, 2020

Conversation

arriolac
Copy link
Contributor

Polygon.kt already contain these extensions but these should be added to List<LatLng> extensions as well.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 18, 2020
@codecov
Copy link

codecov bot commented Mar 18, 2020 •
edited
Loading

Codecov Report

Merging #16 into master will not change coverage by %.
The diff coverage is 0.00%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #16   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           9       9           
  Lines          92      97    +5     
  Branches       18      18           
======================================
- Misses         92      97    +5
Impacted Files Coverage Δ
...rc/main/java/com/google/maps/android/ktx/LatLng.kt 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e72e9cf...47cc464. Read the comment docs.

barbeau
barbeau reviewed Mar 18, 2020
View reviewed changes
Copy link
Contributor

@barbeau barbeau left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A few comments in-line.

Also, is Codecov not working? The bump in coverage in the Java lib of 20% when we moved tests from androidTest to test for Robolectric also seemed suspicious - googlemaps/android-maps-utils#649 (comment).

maps-utils-ktx/src/main/java/com/google/maps/android/ktx/LatLng.kt Outdated
package com.google.maps.android.ktx

import com.google.android.gms.maps.model.LatLng
import com.google.maps.android.PolyUtil
import com.google.maps.android.SphericalUtil

/**
* Computes where the given [latLng] is contained on or near this Polyline within a specified
Copy link
Contributor

Choose a reason for hiding this comment

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

where the given [latLng] is contained on or near -> whether the given [latLng] lies on or near

Copy link
Contributor

Choose a reason for hiding this comment

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

should this be [this]? I'm not sure how this works with Kotlin docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just tested—[this] doesn't work in Kotlin docs

maps-utils-ktx/src/main/java/com/google/maps/android/ktx/LatLng.kt
inline fun List<LatLng>.isLocationOnPath(
latLng: LatLng,
geodesic: Boolean,
tolerance: Double = 0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

0.1 should be DEFAULT_TOLERANCE from the Java lib

maps-utils-ktx/src/main/java/com/google/maps/android/ktx/LatLng.kt
inline fun List<LatLng>.isOnEdge(
latLng: LatLng,
geodesic: Boolean,
tolerance: Double = 0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

0.1 should be DEFAULT_TOLERANCE from the Java lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I meant to do now that AMU 1.0.1 is live on maven. I'll need to bump the version used by the KTX lib and update this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I'll address this as a separate PR (with version bumping AMU to 1.0.1). Will update the PR with doc edits though.

maps-utils-ktx/src/main/java/com/google/maps/android/ktx/LatLng.kt Outdated
): Boolean = PolyUtil.isLocationOnPath(latLng, this, geodesic, tolerance)

/**
* Checks whether or not [latLng] lies on or is near the edge of this polygon within a tolerance
Copy link
Contributor

Choose a reason for hiding this comment

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

tolerance is repeated here - maybe within the [tolerance] (in meters)?

maps-utils-ktx/src/main/java/com/google/maps/android/ktx/LatLng.kt Outdated
): Boolean = PolyUtil.isLocationOnEdge(latLng, this, geodesic, tolerance)

/**
* Computes whether the given point lies inside the specified polygon.
Copy link
Contributor

@barbeau barbeau Mar 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

point -> [latLng]
the specified -> [this]

@arriolac
Copy link
Contributor Author

Also, is Codecov not working?

@barbeau yep, it's not working for some reason. This issue is open #8

@arriolac arriolac merged commit d4cd6f5 into master Mar 18, 2020
@arriolac arriolac deleted the chris/feat/util_ext branch March 18, 2020 23:30
googlemaps-bot pushed a commit that referenced this pull request Apr 2, 2020
@semantic-release-bot
chore(release): 1.0.0 [skip ci]
47fe436 
# 1.0.0 (2020-04-02)

### Bug Fixes

* Add dependencies of maps-utils-ktx in pom. ([#14](#14)) ([d2f14e4](d2f14e4))
* next repositories inside publishing ([38e8dbc](38e8dbc))
* Use api instead of implementation. ([#10](#10)) ([ac40f46](ac40f46))

### Features

* Adding ability to publish to maven central ([#5](#5)) ([5cfa6b1](5cfa6b1))
* Adding KTX to Maps SDK ([#21](#21)) ([3313167](3313167))
* Adding PolyUtil extensions to List<LatLng> ([#16](#16)) ([d4cd6f5](d4cd6f5))
* Create maps-ktx module ([#20](#20)) ([597a70c](597a70c))

### BREAKING CHANGES

* Moved KTX for utils from com.google.maps.android.ktx to
com.google.maps.android.ktx.utils to avoid package name conflicts with
the new maps-ktx module.

* refactor: move core extensions to separate package.

* Using shared version.

* Adjusting settings.gradle.
@googlemaps-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barbeau barbeau barbeau left review comments

Assignees

@barbeau barbeau

Labels
cla: yes This human has signed the Contributor License Agreement. released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arriolac @googlemaps-bot @barbeau @googlebot