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

How about letting CountryCode implement an interface. #53

Closed
mihxil opened this issue Oct 2, 2018 · 10 comments
Closed

How about letting CountryCode implement an interface. #53

mihxil opened this issue Oct 2, 2018 · 10 comments

Comments

@mihxil
Copy link

mihxil commented Oct 2, 2018

I would like that CountryCode implements some 'Region' interface. This would make it possible to make more implementations, like e.g. 'former countries' (I'm setting up something at https://github.com/mihxil/i18n-formerly-assigned).

I suggest something like this:
https://github.com/mihxil/nv-i18n/blob/master/src/main/java/com/neovisionaries/i18n/Region.java

How would you feel about something like this?

@derekmahar
Copy link

I had a similar idea when I created CustomCountryCode as an interface for CountryCode in order to implement class StandardOrUserAssignedCountryCode to accommodate user-assigned country codes.

@GuiSim
Copy link

GuiSim commented May 28, 2019

I'd use this right now for an API that returns XX, which can be used to denote an unknown country (why they didn't use null, I do not know).

@mihxil
Copy link
Author

mihxil commented Jun 6, 2019

I was rather hoping that @TakahikoKawasaki would react how he'd see something like this. I can make a pull request, but may be it's better to know beforehand what kind of reaction to expect.

@mihxil
Copy link
Author

mihxil commented Jun 6, 2019

Actually I was thinking, there is a possible other approach, and I made a new project which provides this 'Region' interface along with a few implementations (one of them or course based on nv-i18n).

So, in this way we actually can add the Region interface without having to add it here. I made it pluggable via java service provider interfaces. So you could simply made a 'RegionProvider' and make it return whatever extra Regions you'd like.

I hacked this up in a few spare hours, so it's not yet quite finished:
https://github.com/mihxil/i18n-regions

@derekmahar
Copy link

I think this approach makes sense. As an enumeration, the existing CountryCode implementation does not allow clients to add new country codes. It is essentially closed to extension. By introducing interface Region, you open CountryCode to extension through custom implementations.

@derekmahar
Copy link

What is the purpose of RegionProvider?

@mihxil
Copy link
Author

mihxil commented Jun 8, 2019

I takes the roles of static methods, like .values() and getByCode. Using java's `java.util.ServiceLoader' to access all registered implementations.

@derekmahar
Copy link

I noticed that https://github.com/mihxil/i18n-regions assumes Java 8, but nv-i18n does not. This would introduce a compatibility issue for clients still using Java 7 and earlier.

@mihxil
Copy link
Author

mihxil commented Jun 9, 2019

Yes, but really? Java 7 was end of life a few years ago, wasn't it? I actually considered making it java 11!

But well, if somebody really considers this an obstacle I'd gladly consider making it java 7 compatible. I suppose that would be relatively simple.

@mihxil
Copy link
Author

mihxil commented Jul 1, 2019

I in the end didn't make it java 7 compatible. The code depended on Stream-api's and I think it would be silly to make everything different just to still be able to support java 7.

It will be java 8+. I tried to even support java 9 module-info.java as good as possible.

@mihxil mihxil closed this as completed Jul 1, 2019
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

No branches or pull requests

3 participants