Repository navigation
Conversation
In some rare cases, the translation might not be present. The fallback code to english was broken due to the `match`. Also, if a country is not a valid iso code, we should display the raw value.
|
Hi @dbaq! Apologies for the delay. I've been away for a bit. Thanks for catching the bug in I'm unsure what to do with invalid country ISO codes though. I have two concerns:
I have empathy for anyone relying on country name data (we did at one point as well) but I'm not convinced that Snail should support it. If the country names are provided by users, the system has no consistency or accuracy. If the names are enumerated, then the system should map them back to ISO codes for processing. Thoughts? I can be persuaded. |
|
hey @cainlevy, just went through my open PR and found this one. It's been a while but we have this PR in production since then. The bug fix for
It is correct, it is a pure design choice. Do you prefer throw an error or a graceful degradation? On my side, I always go try to go for graceful degradations. On our side, why did/do we need it? I migrated 1M addresses from an open country input to a dropdown country list with the iso code as a key. I was able to map 65% of values to an iso code but at the time we still had to support other values. I understand your point of view but I think such a use case could be common and allow to fallback to default formatting and english is decent enough. Let me know your thoughts. |
|
Okay, I've spent some time thinking about this and concluded that graceful degredation is the way to go because that's how the postal system works, too. Sending snail mail is all about making the best of imperfect data. |
cainlevy
left a comment
cainlevy
left a comment
There was a problem hiding this comment.
We'll get this merged yet!
Please let me know if you don't expect to get back to this in the next month or so, and I'll finish up. I'd rather you got credit for contributing, but I realize this has been open for some time and you may or may not still be interested.
| def country=(val) | ||
| @country = Snail.lookup_country_iso(val) | ||
| @country = val | ||
| @country_iso = Snail.lookup_country_iso(val) |
There was a problem hiding this comment.
i'd like to make this change more backwards compatible. let's go with @country = Snail.lookup_country_iso(val) || val and drop the @country_iso ivar. this should mean that behavior will only change for unknown countries: anyone with a standardized data will be unaffected, and anyone with mixed data will hopefully see an improvement in formatting.
| def translated_country(origin, country) | ||
| path = File.join(File.dirname(File.expand_path(__FILE__)), "../assets/#{origin}.yml") | ||
| File.read(path).match(/^#{country}: (.*)$/)[1] | ||
| matches = File.read(path).match(/^#{country}: (.*)$/) |
There was a problem hiding this comment.
i'd love to see this bugfix in a separate commit.
|
Thanks for the review. I'll try to take a look at it soon. |
2 participants
hi @cainlevy,
In some rare cases, the translation might not be present (thinking about AF for instance). The fallback code to english was broken due to the
matchfunction. Also, if a country is not a validiso code, we should display the raw value.