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

add county_source to the sdk #95

Merged
merged 1 commit into from
Nov 15, 2024
Merged

Conversation

maxwellpothier
Copy link
Contributor

@maxwellpothier maxwellpothier commented Nov 7, 2024

ClickUp: https://app.clickup.com/t/86b2rzryh

you may need to run npm run build to build the code before testing.

I added an example address to the example file. To test, run the us_street example and see that the output for county_name is Johnson and the county_fips is 48251.
If you uncomment line 33 in the example, enabling geographic county source, you'll see the output for county_name change to Tarrant and county_fips change to 48439

Once this is approved I'll revert the changes to examples/us_street.js

Copy link
Contributor

@ducanbeu ducanbeu left a comment

Choose a reason for hiding this comment

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

Functionality looks great! Just a couple of changes in the example that I had questions about.

@@ -8,7 +8,8 @@ const Lookup = SmartySDK.usStreet.Lookup;
// const credentials = new SmartyCore.StaticCredentials(authId, authToken);

// for client-side requests (browser/mobile), use this code:
let key = process.env.SMARTY_EMBEDDED_KEY;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it's important we keep this line as it adds context to the comment above. Maybe there could be a comment added after, such as... let key = process.env.SMARTY_EMBEDDED_KEY // "your key".
Also, is there any reason we couldn't freshen this up by changing let -> const? I don't think this variable needs to be mutable, does it?

batch.add(lookup2);
batch.add(lookup3);

await handleResponse(batch);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity... Is there a reason why we don't want to showcase batching in the example anymore?

@maxwellpothier
Copy link
Contributor Author

@ducanbeu
I'll revert the changes in the example file after it's been reviewed, I just left the changes there for ease of testing.

@maxwellpothier maxwellpothier merged commit 8c2bf1b into master Nov 15, 2024
3 checks passed
@maxwellpothier maxwellpothier deleted the max/add-county-source branch November 15, 2024 21:36
@maxwellpothier maxwellpothier restored the max/add-county-source branch November 15, 2024 21:38
@maxwellpothier maxwellpothier deleted the max/add-county-source branch November 15, 2024 21:38
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