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

feat(AgmGeocoder): add geocoder service and tests #1743

Merged
14 commits merged into from
Oct 18, 2019

Conversation

DefJunx
Copy link
Contributor

@DefJunx DefJunx commented Oct 11, 2019

  • add geocoder service
  • add geocoder unit tests
  • add geocoder related typing

implements #1694

@DefJunx DefJunx mentioned this pull request Oct 11, 2019
@codecov
Copy link

codecov bot commented Oct 11, 2019

Codecov Report

Merging #1743 into master will increase coverage by 2.76%.
The diff coverage is 92.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1743      +/-   ##
==========================================
+ Coverage   47.68%   50.45%   +2.76%     
==========================================
  Files          41       43       +2     
  Lines        1839     1881      +42     
  Branches      167      170       +3     
==========================================
+ Hits          877      949      +72     
+ Misses        958      928      -30     
  Partials        4        4
Impacted Files Coverage Δ
packages/core/services.ts 0% <0%> (ø) ⬆️
packages/core/map-types.ts 0% <0%> (ø)
packages/core/services/geocoder-service.ts 100% <100%> (ø)
packages/core/services/google-maps-types.ts 100% <100%> (+100%) ⬆️

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 c16e666...b9503e1. Read the comment docs.

@ghost ghost added the needs: review label Oct 13, 2019
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the contribution. Please go over some things i've pointed out. Thanks.

packages/core/services/google-maps-types.ts Outdated Show resolved Hide resolved
packages/core/services/google-maps-types.ts Outdated Show resolved Hide resolved
packages/core/services/google-maps-types.ts Outdated Show resolved Hide resolved
packages/core/services/google-maps-types.ts Outdated Show resolved Hide resolved
packages/core/services/google-maps-types.ts Outdated Show resolved Hide resolved
packages/core/services/geocoder-service.ts Outdated Show resolved Hide resolved
packages/core/services/geocoder-service.spec.ts Outdated Show resolved Hide resolved
packages/core/services/geocoder-service.spec.ts Outdated Show resolved Hide resolved
packages/core/services/geocoder-service.spec.ts Outdated Show resolved Hide resolved
@DefJunx
Copy link
Contributor Author

DefJunx commented Oct 17, 2019

Hi @doom777, thank you very much for your feedback. It really helps me improving my code.
I'll make sure to address the issues you've pointed out and fix them.

DefJunx pushed a commit to DefJunx/angular-google-maps that referenced this pull request Oct 17, 2019
- Export new map types in the correct place

Addresses PR sebholstein#1743 review
@DefJunx
Copy link
Contributor Author

DefJunx commented Oct 17, 2019

@doom777 i've done the required changes and pushed them to the branch.

@DefJunx DefJunx requested a review from a user October 17, 2019 13:16
@DefJunx
Copy link
Contributor Author

DefJunx commented Oct 17, 2019

@doom777 I also rebased together all the commits, leaving the implementation message.

Again, you might hate me after all these notifications but please don't!

@dockleryxk
Copy link

Just wanted to say this looks really good. Thanks for doing it!

- add geocoder service
- add interfaces to support geocoder request and response
- add test cases

Implements sebholstein#1694
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

just one more change. it didnt work for me without it.

packages/core/services/geocoder-service.ts Outdated Show resolved Hide resolved
Daniele Cencig added 7 commits October 17, 2019 18:55
- add geocoder service
- add interfaces to support geocoder request and response
- Add check to see if the geocoder instance is defined;
if not, return an empty array of results
- add initial test implementation

implements SebastianM#1694
- Fix wrong geocodeMock jest function

implements SebastianM#1694
- avoid method being called without google being initialized
- Add geocoder interface

Implements sebholstein#1694
- Changed geocoder service structure

Implements sebholstein#1694
- add geocoder object as private observable
to follow style of other services

Implements sebholstein#1694
@ghost
Copy link

ghost commented Oct 17, 2019

hey, you missed my latest change request

hero2frag@gmail.com added 2 commits October 17, 2019 19:16
@DefJunx
Copy link
Contributor Author

DefJunx commented Oct 17, 2019

@doom777 no no, I saw it. though I had a fight with git because for some reason, when I pulled my changes on another machine I screwed everything up so I had to do some work to fix it.

Let me know if everything is in place now, it should be I think.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I came upon an efficiency problem. Solution is in the comments.

packages/core/services/geocoder-service.ts Outdated Show resolved Hide resolved
packages/core/services/geocoder-service.spec.ts Outdated Show resolved Hide resolved
and patch tests

- Remove unwanted test expectation
- Change geocoder Observable implementation to avoid instantiating
multiple geocoder object
Daniele Cencig added 3 commits October 18, 2019 10:11
- Remove unwanted test expectation
- Change geocoder Observable implementation to avoid instantiating
multiple geocoder object
- Simplify creation of connectableGeocoder observable
@ghost ghost merged commit 3549ccf into sebholstein:master Oct 18, 2019
@DefJunx DefJunx deleted the geolocator-service branch October 18, 2019 09:27
pertsenga pushed a commit to pertsenga/angular-google-maps that referenced this pull request Nov 6, 2019
- add geocoder service
- add interfaces to support geocoder request and response
- add test cases

Implements: sebholstein#1694
@Waniusza Waniusza mentioned this pull request Nov 18, 2019
This pull request was closed.
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.

2 participants