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

Bulk Geocode - Properly pass thorugh params #633

Merged
merged 6 commits into from
Oct 30, 2019
Merged

Bulk Geocode - Properly pass thorugh params #633

merged 6 commits into from
Oct 30, 2019

Conversation

gavinr
Copy link
Contributor

@gavinr gavinr commented Oct 22, 2019

@codecov
Copy link

codecov bot commented Oct 23, 2019 •
edited
Loading

Codecov Report

Merging #633 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #633   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         102    102           
  Lines        1512   1513    +1     
  Branches      260    260           
=====================================
+ Hits         1512   1513    +1
Impacted Files Coverage Δ
packages/arcgis-rest-geocoding/src/bulk.ts 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 6d59e57...f21bf49. Read the comment docs.

@gavinr gavinr marked this pull request as ready for review October 23, 2019 14:41
@gavinr gavinr requested a review from tomwayson October 23, 2019 16:12
@gavinr
Copy link
Contributor Author

gavinr commented Oct 23, 2019 •
edited
Loading

@jgravois would love your feedback on this code change if you have time - i'm pretty sure it's working now, but want to make sure it's done in the best way.

jgravois
jgravois approved these changes Oct 23, 2019
View reviewed changes
Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

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

👍

tomwayson
tomwayson approved these changes Oct 24, 2019
View reviewed changes
Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

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

If it's good enough for @jgravois it's good enough for me.

@jgravois
Copy link
Contributor

I'm glad you didn't tell him to get clever and do:

options.params.addresses = {
  records: options.addresses.map(attributes => { attributes })
};

#forEach()4lyfe

@tomwayson
Copy link
Member

Actually @gavinr do you mind using map as @jgravois suggests above and then doing npm run c to generate a commit message that will make it easier for me to generate the changelog? then I will merge and deploy.

@tomwayson tomwayson merged commit 9e755b7 into Esri:master Oct 30, 2019
@tomwayson
Copy link
Member

Thanks @gavinr, I'll get this released later today.

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

@tomwayson tomwayson tomwayson approved these changes

@jgravois jgravois jgravois approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gavinr @jgravois @tomwayson @gavinr-maps