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 iso to fuzzy dont #58

Merged
merged 3 commits into from
Oct 16, 2024
Merged

Add iso to fuzzy dont #58

merged 3 commits into from
Oct 16, 2024

Conversation

b-j-mills
Copy link
Contributor

I created a new function to parse the admin_fuzzy_dont values. I made the iso code optional so it would be backward compatible with the values we've already added and we wouldn't have to track down the country each one came from.

@b-j-mills b-j-mills requested a review from mcarans October 16, 2024 16:08
Copy link

github-actions bot commented Oct 16, 2024

Test Results

30 tests  ±0   30 ✅ ±0   1m 20s ⏱️ ±0s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 2445f24. ± Comparison against base commit 332f867.

♻️ This comment has been updated with latest results.

@coveralls
Copy link

coveralls commented Oct 16, 2024

Coverage Status

coverage: 96.104% (+0.04%) from 96.066%
when pulling 2445f24 on add_iso_to_fuzzy_dont
into 332f867 on main.

Copy link
Contributor

@mcarans mcarans left a comment

Choose a reason for hiding this comment

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

This looks fine

if value not in relevant_admin_fuzzy_dont:
relevant_admin_fuzzy_dont.append(value)
continue
prefix, name = value.split("|")
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 it would be better if this matched by parent as well (where parent could be adm1 for adm2 for example), but if you need this urgently then this could be changed in a separate PR. What do you think? That is the logic in get_admin_name_replacements

@b-j-mills b-j-mills merged commit ff4738c into main Oct 16, 2024
6 checks passed
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.

3 participants