-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
language code support #81
base: master
Are you sure you want to change the base?
Conversation
DanielHilton
commented
Nov 13, 2024
- Adding ISO-639-1 language code support
- Adding tests
✅ Deploy Preview for biter777countries ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@biter777 i'd welcome your feedback on this PR! 😄 |
countries.go
Outdated
case BEL: | ||
return LanguageNL |
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'm sorry, but there is a problem with them method signature
Some countries have multiple official languages
Here's a list of countries with multiple official languages:
- Africa: South Africa, Algeria, Burundi, Cameroon, Cape Verde, Central African Republic, Comoros, Djibouti, Eritrea, Equatorial Guinea, Kenya, Lesotho, Madagascar, Morocco, Mauritius, Mauritania, Namibia, Uganda, Rwanda, Seychelles, Swaziland, Tanzania, Chad
- Americas: Canada, Bolivia
- Asia: India, Singapore, Fiji
- Europe: Belgium, Switzerland
Your method should return a list of languages
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.
that's fair, i can make these adjustments
One question. Your additions are related to languages. Except the mention of the country in a few methods. I'm unsure it should be added to this repository. I mean you could have created your own package dedicated to languages, that is could return its own constants. Your code is good, and highly inspired from country one. But, this repository is about countries I'm raising the point because I find strange to add languages (another iso norm) to a repository focused on country one |
Also, have you considered existing packages available? Here are the few I found https://github.com/emvi/iso-639-1 Implementing methods to get the languages from a country could be addee in these packages |
package countries | ||
|
||
import ( | ||
"encoding/json" | ||
"fmt" | ||
) | ||
|
||
type LanguageCode string // string for database/sql/driver.Valuer compatibility | ||
|
||
type Language struct { | ||
Name string | ||
Code LanguageCode | ||
} | ||
|
||
func (LanguageCode) Type() string { | ||
return TypeLanguageCode | ||
} | ||
|
||
func (lc LanguageCode) String() string { //nolint:gocyclo | ||
switch lc { | ||
case LanguageAA: |
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 would move all this to a sub package
This way, it's clearer for people who imports
Also, it avoids you to prefix everything related to language with language
It would look like this
package countries | |
import ( | |
"encoding/json" | |
"fmt" | |
) | |
type LanguageCode string // string for database/sql/driver.Valuer compatibility | |
type Language struct { | |
Name string | |
Code LanguageCode | |
} | |
func (LanguageCode) Type() string { | |
return TypeLanguageCode | |
} | |
func (lc LanguageCode) String() string { //nolint:gocyclo | |
switch lc { | |
case LanguageAA: | |
package languages | |
import ( | |
"encoding/json" | |
"fmt" | |
) | |
type Code string // string for database/sql/driver.Valuer compatibility | |
type Language struct { | |
Name string | |
Code Code | |
} | |
func (Code) Type() string { | |
return TypeLanguageCode | |
} | |
func (lc LanguageCode) String() string { //nolint:gocyclo | |
switch lc { | |
case CodeAA: |
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.
Whilst that is a fair point, that is not the way this package is written. There are other facets of a country which are all under the countries
package.
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.
You are right. Currencies are aside countries while it's another iso norm
This repo concerns itself with facets or properties of countries. This includes TLDs, dialing codes, regions and more. It stands to reason that when dealing with countries, it may be of use to include the language information about the country you are working with. This is not merely an ISO standard repo, as such I think it is relevant to be able to call |
Please note, I'm not against the addition. I raised a point to make sure and you replied I like your PR. |
@@ -181,6 +181,7 @@ const ( | |||
LanguageTatar LanguageCode = "tt" | |||
LanguageTwi LanguageCode = "tw" | |||
LanguageTahitian LanguageCode = "ty" | |||
LanguageTuvaluan LanguageCode = "tvl" |
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.
tvl is not available as an iso-code in iso 639-1, only in iso 639-2 alpha-3
You shouldn't mention here, or use 3 letter code everywhere (better option)