Replace Country class with enumeration

[frenzibyte]

One step towards supporting inclusion of user country in local scores. Countries have been copied from the countries.json map with help from VIM keybindings, which will be removed in a follow-up PR given the current size of this PR and the usage behind that file.

Reason for changing it to enum is to make it easier to store the user's country in local scores, by only specifying the acronym rather than passing the full Country structure and making a Realm version of it which inherits EmbeddedObject. It is also a good code-quality change overall seeing how it reflects on the test scenes.

[frenzibyte]

I initially went towards using null as a default/unknown country value, but wasn't sure if that's a good direction code-wise.

I was also not sure between using Country.XX and default, but decided to use the latter as it reads somewhat better. Open for argument.

[frenzibyte]

Could alternatively rename __.png in resources project to XX.png rather than doing this, but didn't want to initiate another game resources release just for this.

[frenzibyte]

Would hope to get rid of this eventually and have osu-web provide the country code in a country_code property on the user object directly.

[bdach]

Not super sure I agree with this direction, conceptually speaking? The list of countries in this world isn't exactly that immutable. With this enumeration existing, now we're on the hook for tracking geopolitical changes and keeping the set of countries up-to-date in the enum, which was basically a non-issue in the previous structure.

At least #19139 has enough sense to persist the enum as a string to DB, but at the point that that workaround has to exist, I am sort of questioning what exactly the enum is giving us here.

I don't disagree with only persisting the ISO country code and having a mapping function to unmap that to a country name - that is complexity we will likely not avoid - but I am against countries being an enum.

[frenzibyte]

Not super sure I agree with this direction, conceptually speaking? The list of countries in this world isn't exactly that immutable. With this enumeration existing, now we're on the hook for tracking geopolitical changes and keeping the set of countries up-to-date in the enum, which was basically a non-issue in the previous structure.

At least #19139 has enough sense to persist the enum as a string to DB, but at the point that that workaround has to exist, I am sort of questioning what exactly the enum is giving us here.

It's one way out of many to be able to take ISO country code and map it to a country name, without having to include the full name itself inside DB, which is the goal of this PR.

I don't disagree with only persisting the ISO country code and having a mapping function to unmap that to a country name - that is complexity we will likely not avoid - but I am against countries being an enum.

Alternative directions could be to change it to a dictionary, or an extensions method which takes a code and returns the full name for it. But I can't see any difference from an enum (if not just being more complex) since we're still bound to update it whenever the list of countries change? I don't see why you are against having it as an enum?

[peppy]

Enum gives us "type safety" for tests etc. I'm neutral on enum vs acronym strings though. Whatever works really.

[bdach]

If the country is in an enum as you have it, you have to both track new countries appearing and disappearing (by adding them to the enum), you have to track countries changing name (which happens - see Czech Republic -> Czechia, FYR Macedonia -> North Macedonia, Turkey -> Türkiye), and you have to unmap ISO codes to the enum in all cases the data is being retrieved/persisted as a string (API responses, DB storage).

If the country is represented by a string of its ISO code, you only have to worry country changing names. New countries appearing and old disappearing are mostly handled automatically, and there is no unmapping step required. How would this be more complex?

[peppy]

End of the day, we want to use the acronyms. That's standard across infrastructure. Not sure if that matches ISO code, but just going to add that.

[frenzibyte]

If the country is represented by a string of its ISO code, you only have to worry country changing names. New countries appearing and old disappearing are mostly handled automatically

You will still have to update the mapping method to handle those new countries. But the point about not having to unmap is fair, okay. So we're all agreeing on a CountryExtensions.ToCountryName(string code) extension method?

End of the day, we want to use the acronyms. That's standard across infrastructure. Not sure if that matches ISO code, but just going to add that.

If that matches with what the enum members are currently named, then yeah.

[bdach]

You will still have to update the mapping method to handle those new countries

Way I see it, the chance of us missing the fact that some new country has been added is 99.9%. At that point I don't much care how wrong the end result is going to be, because we won't see it until someone has actually reported it already. It probably shouldn't crash the game, but falling back to "Unknown" as the country name is probably okay enough (and not much different to what your current code would do).

So we're all agreeing on a CountryExtensions.ToCountryName(string code) extension method?

As I stated above, this will unfortunately have to exist any way we cut it, so yes.

[frenzibyte]

After spending some time on moving away from enum, I've came across two ways:

• Remove Country entirely and rely on the code as the country data type: master...frenzibyte:osu:country-string
• Keep Country as class with private instantiation and rely on helper method: master...frenzibyte:osu:country-helper

I started with making it string all the way, but then started to realize how bad that looks in a strongly-typed environment. Therefore I went with the other way which is a Country class with private instantiation, with JSON properties still kept for compatibility with current API structure. @bdach interested in your opinion before I go ahead and PR one of these.

On the realm side of things, with the first approach only a string CountryCode property will be added, but with the second approach it'll look something like this:

public Country? Country
{
    get => Country.FromCode(CountryCode);
    set => CountryCode = value.Code;
}

[MapTo(nameof(Country))]
public string CountryCode { get; set; }

[peppy]

• If we're making Country objects they have to be structs.

On the realm side of things, with the first approach only a string Country property will be added

If this implies storing the acronym, it should be called CountryAcronym. If it implies storing the full name then that can't be a valid solution.

I have no issue with any of the provided solutions, including the enum. Choose whichever allows this to be merged.

[frenzibyte]

If this implies storing the acronym, it should be called CountryAcronym.

I meant the country acronym/code, yeah.

[bdach]

Upon seeing the branches linked above I withdraw my objections. Between the three variants I see no clear best option, they all have varying degrees on wonkiness, so may as well go with what's PRed already to avoid bikeshedding further.

[peppy]

Inclined to go with the enum then, since that's what we already have here. Still not happy with the enum-string mapping in realm, but also hoping that realm will eventually support enum types properly and we can drop that everywhere.

[peppy]

Not 100% on the use of GetDescription to get the country name. It feels a bit too generic of a method call? Guess it works for now though. If it ever comes up as an issue we could make an extension method to use instead. Or maybe reconsider whether enum is the best fit.