-
Notifications
You must be signed in to change notification settings - Fork 0
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 CHECK_NAME_DUPLICATES
feature
#33
add CHECK_NAME_DUPLICATES
feature
#33
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please clean up the comments.
handler.ts
Outdated
{ customResponse: cur, primaryResponse: prev }, | ||
// Default to false | ||
CHECK_NAME_DUPLICATES !== 'false' | ||
// convertQSPToGeocoderArgs(event.queryStringParameters)?.focusPoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How hard would it be to add a test for this functionality?
@@ -112,13 +112,15 @@ describe('response merging', () => { | |||
customResponse: CUSTOM_RESPONSE, | |||
primaryResponse: GEOCODE_EARTH_RESPONSE | |||
}, | |||
true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get a test that tests this with false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried but couldn't do it in any way that made sense. This test tests for something completely different. We could add an entire new test case but this is a really weird edge case so I'm tempted to add a TODO here instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be fine with a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! You can add the todo if you want but I think it probably doesn't matter. We'll probably never get to it anyway.
Adds a new feature
CHECK_NAME_DUPLICATES
in the config which disables name-based duplicate checking. This is helpful when your GTFS has common names in it.