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

Rahul's request #22

Merged
merged 4 commits into from
Jan 23, 2014
Merged

Rahul's request #22

merged 4 commits into from
Jan 23, 2014

Conversation

rahulpratapm
Copy link
Contributor

No description provided.

Supporting queries like "Macau, China" which wouldn't work until this point because Macau has its own country code and doesn't directly related to China in the geonames hierarchy. This prevented Macau, China from being considered a valid parse.
@@ -192,6 +192,15 @@ trait NameUtils {
.toList
def isFeatureBlacklistedforParent(id: Long) = blacklistedParentIds.has(id)

private val dependentCountryRelationships =
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this belongs in NameUtils. The feature-parent-blacklist is here because it affects name formatting, but this affects parsing itself. Just putting it in GeocodeImpl.scala would be cleaner or it's own file/object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to new file CountryUtils.scala

1. Creating separate CountryUtils object and file
2. Adding dependent country support to AutocompleteGeocoderImpl.scala, requiring inverse dependent country lookup and country code to geonames id lookup (from countryInfo.txt)
3. Minor correctness/completeness bug fixes
4. Name, style changes
if (req.debug > 0) {
logger.ifDebug("checking if %s is an unused parent of %s",
fid, parse.map(_.fmatch.longId))
}

val isValid = (parse.exists(_.fmatch.scoringFeatures.parentIds.has(fid)) ||
parse.exists(_.fmatch.scoringFeatures.extraRelationIds.has(fid))) &&
parse.exists(_.fmatch.scoringFeatures.extraRelationIds.has(fid)) ||
(featureMatch.fmatch.feature.woeType == YahooWoeType.COUNTRY && parse.exists(p => CountryUtils.getDependentCountryCodesForCountry(fcc).has(p.fmatch.feature.cc)))) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

please wrap lines at 80 or 100, 120 if it's really really necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a CountryUtils.doFeatureCountriesMatch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

blackmad added a commit that referenced this pull request Jan 23, 2014
@blackmad blackmad merged commit e46a976 into foursquare:master Jan 23, 2014
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