Skip to content

fix: allow using existing session token(String) in place autocomplete#722

Merged
arriolac merged 2 commits intogooglemaps:mainfrom
SethMyers:allow-using-session-token-place-autocomplete
May 17, 2021
Merged

fix: allow using existing session token(String) in place autocomplete#722
arriolac merged 2 commits intogooglemaps:mainfrom
SethMyers:allow-using-session-token-place-autocomplete

Conversation

@SethMyers
Copy link
Copy Markdown
Contributor

Fixes #689

@google-cla
Copy link
Copy Markdown

google-cla Bot commented Feb 16, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla Bot added the cla: no This human has *not* signed the Contributor License Agreement. label Feb 16, 2021
@SethMyers SethMyers changed the title 689 allow using existing session token(String) in place autocomplete fix: allow using existing session token(String) in place autocomplete Feb 16, 2021
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #722 (9907e11) into master (b0a48af) will decrease coverage by 0.06%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #722      +/-   ##
============================================
- Coverage     69.11%   69.05%   -0.07%     
  Complexity      531      531              
============================================
  Files           129      129              
  Lines          2888     2895       +7     
  Branches        284      283       -1     
============================================
+ Hits           1996     1999       +3     
- Misses          739      744       +5     
+ Partials        153      152       -1     
Impacted Files Coverage Δ Complexity Δ
...ain/java/com/google/maps/DirectionsApiRequest.java 67.56% <ø> (ø) 24.00 <0.00> (ø)
...java/com/google/maps/DistanceMatrixApiRequest.java 53.84% <ø> (ø) 13.00 <0.00> (ø)
src/main/java/com/google/maps/GeoApiContext.java 61.07% <0.00%> (-0.84%) 18.00 <0.00> (ø)
...java/com/google/maps/PlaceAutocompleteRequest.java 58.82% <0.00%> (-5.70%) 12.00 <0.00> (ø)
src/main/java/com/google/maps/DirectionsApi.java 86.36% <100.00%> (+5.41%) 3.00 <0.00> (ø)
...main/java/com/google/maps/errors/ApiException.java 39.13% <100.00%> (ø) 9.00 <0.00> (ø)
...c/main/java/com/google/maps/model/AddressType.java 100.00% <100.00%> (ø) 6.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 3650a60...9907e11. Read the comment docs.

@SethMyers
Copy link
Copy Markdown
Contributor Author

@googlebot I signed it!

@google-cla google-cla Bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Feb 16, 2021
@SethMyers SethMyers force-pushed the allow-using-session-token-place-autocomplete branch from 9907e11 to 20b77c8 Compare February 16, 2021 19:08
/**
* Construct a session that is a continuation of a previous session.
*
* @param token The unique token to be used as the identifier for this session.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would expect token here to be a string representation of a UUID after reading this comment, but that's not the case given the implementation. While this provides convenience, I think this is potentially error-prone. I suggest either clarifying the comment, or keeping the existing API of constructing a SessionToken from a UUID. The latter seems like a clearer API to me, but if a convenience initializer is useful for others, I'm happy to merge in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

would
@param token The character string to be used as the unique identifier for this session.
be clear enough?

Copy link
Copy Markdown

@vojtapol vojtapol May 17, 2021

Choose a reason for hiding this comment

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

keeping the existing API

The existing API does not work well if the session identifier is not a UUID.

@arriolac arriolac changed the base branch from master to main March 24, 2021 18:50
@SethMyers SethMyers requested a review from arriolac May 17, 2021 15:33
@arriolac arriolac merged commit 7b0690a into googlemaps:main May 17, 2021
googlemaps-bot pushed a commit that referenced this pull request May 18, 2021
## [0.18.1](v0.18.0...v0.18.1) (2021-05-18)

### Bug Fixes

* allow using existing session token(String) in place autocomplete ([#722](#722)) ([7b0690a](7b0690a))
* Run release on main. ([#731](#731)) ([ffbfd7a](ffbfd7a))
arriolac pushed a commit that referenced this pull request May 18, 2021
* allow using existing session token(String) in place autocomplete ([#722](#722)) ([7b0690a](7b0690a))
* Run release on main. ([#731](#731)) ([ffbfd7a](ffbfd7a))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow using existing session token(String) in place autocomplete

4 participants