Implement pluralisable strings#6734
Conversation
4253cee to
9d838b2
Compare
|
@bdach See if you find this more digestible. I'm certain it's the correct way to go, save for better documentation/commenting/naming/etc - let me know if this is inadequate as is (was a 20 minute implementation). |
If you want my honest thoughts... Before I even look at any of these PRs in terms of the code inside, the first metric I'm going to use when evaluating a change like this is to look at the usage code. And...
This kinda sucks? It sucks for two reasons.
Now I have no immediate answers to either of these criticisms. If I had them, I'd have PR'd this sort of change myself ages ago, and I'm pretty sure I had at least one go at this before hitting these exact walls and bailing since I wasn't sure I could pull it off. Maybe you can bring a counterexample of existing web strings that make your path clearly the only viable one, given how confident you appear to be in saying it's the "correct way to go", but right now I don't see it so I really do wonder if this is the best that can be done. Also I do note that both of the predecessor PRs suffer from issue (1) above, as far as I can tell - but in theory not (2). I say "in theory" because none of the previous authors have actually tried to follow up. |
I can imagine use cases where you don't necessarily want the same number to become an argument with whatever formatting the framework so chooses to use.
Yes indeed, I didn't want to do this.
Emphasis mine. I think it's important to understand this limits the potential scope of localisation. You'd no longer able to cleanly compose strings for example. Simple example but let's say the pluralisable string was As for (2), making it return a pluralisable string can always be done as a followup. None of the code here would change as far as I can tell. The question is detecting when exactly that's the case, and I'm not sure that can be done safely without potentially mangling strings, so I'm not going to attempt it for the time being. I mean, have a look at how the strings are actually defined: https://github.com/ppy/osu-web/blob/21a7e87a2eeb504668a0877cd6b3a258d2e8e144/resources/lang/en/common.php#L69-L87 There's no indication of "this is a string that's pluralisable" other than it having a delimiter in the string - you can't even count on it having a Some strings (e.g. |
This is a concern that I don't see in scope because the grammar rules can change yet again for decimals so it just wouldn't work anyhow without further nudging of the pluralisation rules. Counterexample: in Polish, the proper grammatical form for some nouns when decimals come into play is a completely different one than any of the integral-plural variants:
Whether this could be eventually supported is up for debate. I'd say we don't want to. My mindset was to basically chain framework to the scope of pluralisation as done in laravel and move no further, because I don't want to find myself suddenly maintaining a localisation library and needing to research how fractional plural noun forms work in like Swedish or something.
This is fair. I expect it to be the 5% case, but it is real. I was hoping that to cover the 95% case, maybe a convenience helper could be provided. Something along the lines of public static PluralisableString ToQuantity(this Func<LocalisableString, LocalisableString> strTemplate, int quantity, char separator = '|')
=> new PluralisableString(strTemplate(quantity.ToLocalisableString()), quantity, separator);With the end goal of writing something like this: var t = CommonStrings.CountMinutes.ToQuantity((int)difference.TotalSeconds);But compiler says no. You'd either have to wrap the prefix into a Maybe could be done with further support osu-resources-side (to expose overloads of relevant methods that return the
Yeah this is also bad for my best case scenario. All which to say, I'm like 80% sold on this being the-best-that-can-be-done. That said I won't do a proper review until Monday. |
|
We could do the following because after all we have some static helpers: diff --git a/osu.Framework/Localisation/LocalisableString.cs b/osu.Framework/Localisation/LocalisableString.cs
index be2dde5fd3..0df7b1e375 100644
--- a/osu.Framework/Localisation/LocalisableString.cs
+++ b/osu.Framework/Localisation/LocalisableString.cs
@@ -4,6 +4,7 @@
using System;
using System.Diagnostics.CodeAnalysis;
using JetBrains.Annotations;
+using osu.Framework.Extensions.LocalisationExtensions;
namespace osu.Framework.Localisation
{
@@ -47,6 +48,8 @@ public LocalisableString(ILocalisableStringData data)
/// <param name="interpolation">The interpolated string containing format and arguments.</param>
public static LocalisableString Interpolate(FormattableString interpolation) => new LocalisableFormattableString(interpolation);
+ public static LocalisableString Quantity(Func<LocalisableString, LocalisableString> strTemplate, int quantity) => strTemplate(quantity.ToLocalisableString()).ToQuantity(quantity);
+
/// <summary>
/// Indicates whether the specified <see cref="LocalisableString"/> is <see langword="null"/> or an empty <see langword="string"/>.
/// </summary>
Usage would then be: LocalisableString.Quantity(osu.Game.Resources.Localisation.Web.CommonStrings.CountBadges, 5);That said, I'm still against this because I believe it leads localisation into more potentially unexpected results - if you ever use this in a context where that parameter is not a quantity, things are going to mess up. I highly prefer being explicit about it. I'll grit my teeth if that's what it takes to get this over the line. |
|
I took a bit of time to implement pluralisable strings in Full diff of osu-resources: https://gist.github.com/smoogipoo/5d03d91d1e4984ed66b76e1908eb1748 Again I want to say that this can be done at any point in time and I don't feel any rush in doing this. |
bdach
left a comment
There was a problem hiding this comment.
Pushed minor cleanups, docs & attributions. Seems okay. @smoogipoo plz double-check.
peppy
left a comment
There was a problem hiding this comment.
Just a quick check, looks okay to me!
Supersedes / closes #6733
Supersedes / closes #4918
Used tests + some implementation from first PR above, with my proposal.
Example in-game usage:
Named it
ToQuantity()to match humanizer.