Closed TylerBrinkley closed 4 years ago
When a named argument is not found a FormatException
should be thrown but using the Func<string, object>
parameter doesn't provide a way of returning whether the named argument was found or not making it the responsibility of the consumer to throw the Exception
. To fix this a TryGetValue
delegate could be added with an out value
parameter and returns whether the named argument was found. Alternatively the Func
could return a combined success and value object or simply a ValueTuple
. Any thoughts?
I've decided to go with ValueTuple
and have updated the proposal accordingly.
@AlexGhiondea, @joperezr any thoughts on this proposal?
Hey @TylerBrinkley thanks a lot for your proposal. @AlexGhiondea and I have triage tomorrow, so we will go through proposals and add our feedback 😄 Talk to you soon.
@joperezr Excellent, thanks. I appreciate it.
@joperezr just checking if you guys got the chance to look over this proposal?
so sorry about that, we've been triaging issues and haven't got into yours yet, but we are very close!
@TylerBrinkley as @joperezr has mentioned we haven't yet got around triaging this.
However, I was looking at this now in preparation for triage, and can't figure out why I can't rewrite the code above like this? Can you help me understand?
DateTime time = DateTime.Now;
LogLevel logLevel = LogLevel.Error;
string userId = "admin";
string message = "Poor error message";
string logFormat = "{time:yyyy/MM/dd HH:mm:ss} - {logLevel} ({userId}): {message}";
@AlexGhiondea I should have been more clear in my example, the logFormat
is coming from configuration. The value I gave it was just an example. I've updated the proposal example to retrieve the value from configuration.
@TylerBrinkley thanks for the updated example.
In the updated example you have, putting the format string in the resources will actually strongly couple the string to the code. I don't think that is something you want in general.
In your example, if you rename the values in your code, you will have to remember to go and change it in the resource strings as well. And you also have to rename them in all of the localized strings as well.
On top of that, you need to build a different parser in the string.Format code which IMO is not trivial to write.
I think there are other ways you can solve the problem you are pointing out that do not require changes to the string class.
@AlexGhiondea thanks for taking the time to evaluate this proposal.
You're right that it strongly couples the argument names to the code however so does the standard string.Format
as it requires you to know the order of the arguments passed into the code.
Here's an example implementation. Note, I haven't tested this yet.
public static string Format(string format, IDictionary<string, object> valueProvider)
{
if (valueProvider == null)
{
throw new ArgumentNullException(nameof(valueProvider));
}
return Format(format, name => valueProvider.TryGetValue(name, out var value) ? (true, value) : (false, null));
}
public static string Format(string format, Func<string, (bool found, object value)> valueProvider)
{
if (format == null)
{
throw new ArgumentNullException(nameof(format));
}
if (valueProvider == null)
{
throw new ArgumentNullException(nameof(valueProvider));
}
var sb = new StringBuilder(format.Length);
var namedArgsDictionary = new Dictionary<string, (int Index, object Value)>();
for (var i = 0; i < format.Length; ++i)
{
var c = format[i];
sb.Append(c);
if (c == '{')
{
++i;
var startName = i;
var endName = -1;
var done = false;
for (; i < format.Length && !done; ++i)
{
switch (format[i])
{
case '{':
if (i == startName)
{
sb.Append('{');
done = true;
break;
}
if (endName == -1)
{
throw new FormatException("Named argument cannot contain '{'");
}
break;
case ':':
case ',':
if (endName == -1)
{
endName = i;
}
break;
case '}':
if (endName == -1)
{
endName = i;
}
if (endName == startName)
{
throw new FormatException("Format string cannot contain an empty named argument");
}
var name = format.Substring(startName, endName - startName);
int index;
if (namedArgsDictionary.TryGetValue(name, out var tuple))
{
index = tuple.Index;
}
else
{
var (found, value) = valueProvider(name);
if (!found)
{
throw new FormatException($"Could not find named argument \"{name}\"");
}
index = namedArgsDictionary.Count;
namedArgsDictionary.Add(name, (index, value));
}
sb.Append(index);
for (var j = endName; j <= i; ++j)
{
sb.Append(format[j]);
}
done = true;
break;
}
}
if (!done)
{
throw new FormatException("Format string missing a named argument closing bracket");
}
--i;
}
}
var args = new object[namedArgsDictionary.Count];
foreach (var pair in namedArgsDictionary)
{
var tuple = pair.Value;
args[tuple.Index] = tuple.Value;
}
return string.Format(sb.ToString(), args);
}
@TylerBrinkley thanks for the sample implementation. One thing that came to mind is - does it handle escaping characters? I worry that there are quite a few corner cases when parsing the string fomat that we need to make sure we cover.
so does the standard string.Format as it requires you to know the order of the arguments passed into the code.
That's true -- however, the coupling is looser when using argument order:
'{time}'
. You don't have to tell them not to localize '{0}'
'{time}'
and when not to localize it.It handles the case of two opening braces which denotes that an opening brace should be output as well as the case of two closing braces which denotes that a closing brace should be output. It doesn't allow an opening brace in names. All it really does is identifies named arguments and replaces them with an indexed argument and then has the standard string.Format
do the rest of the parsing. I think it should cover all of the edge cases.
Otherwise you need to figure out a way to tell people not to localize '{time}'. You don't have to tell them not to localize '{0}'
I suppose that's true but I think it should be obvious given that it's enclosed in braces.
Bottomline, I think using named arguments is a big win in readability in the resource file compared to argument positions and mirrors string interpolation very well.
@TylerBrinkley thanks for confirming that we do some escaping -- did not have a change to do a full review of the code.
I am not sure that people will actually know when to localize some words and when they should not. I think for small projects it is probably obvious, but in larger ones, where the localization is done by a different team I think it is not going to be easy to tell when a word is a variable name and when it is a word that should be translated (but it happened to be in curly braces for display purposes).
I agree that it makes the resource file easier to read, but I think it comes with quite a few disadvantages as well (which I have listed above) and I don't believe this should be added to the string type.
@weshaggard @terrajobst what do you guys think?
Another use case for this is as the basis of a basic templating system where you don't have to pass in all available values but just use the valueProvider
.
Looked at this on triage again, and as mentioned above we don't feel there is enough value given the disadvantages that it could bring. Lets get @weshaggard and @terrajobst opinion on it.
I tend to agree that this doesn't add enough value to be added to string directly. String.Format is already a huge source of bugs and performance issues and adding more layers is not something we really want to do. I like the general idea of a basic templating system but I think it would be better served as a higher level concept not directly on string.
Given the previous feedback I'm going to close this for now, we do however appreciate the contribution. Also, feel free to re-open if you come up with new scenarios that might add value to this proposal.
Thanks for the consideration!
may i know what's going on about this function ?
may i know what's going on about this function ?
This issue has been closed for two years.
@stephentoub So this function is on hold? or give up?
As described in the comments above, we decided it's not worthwhile.
@stephentoub IMO, even if don't do anything for string.Format
, C# should have its own templating mechanism. look SmartFormat.Net downloads count, this is absolutely a broad demand.
C# should have its own templating mechanism.
There's string interpolation; and FormattableString though for C# language enhancements you might want to propose it in: https://github.com/dotnet/csharplang ?
@benaadams Interestingly, when i was there, i was advised to come here. both of them not enough for a complete template function, you know that.
String interpolation has proven to be an extremely useful feature in
C#6
but sometimes I find myself needing the string format to be configurable and as such am not able to use string interpolation as it requires the format to be evaluated at compile time. I propose the addition ofString.Format
overloads that will allow users to use named arguments instead of argument indexes similar in appearance to string interpolation.Rationale and Usage
With this implemented what used to be this
can now become this
which will be much easier to understand and maintain in configuration.
An additional use case for this is as the basis for a basic text templating system where the values can be dynamically retrieved using the
Func<string, (bool found, object value)>
overload.Proposed API
Details
string.Format
but replace named arguments with indices.Ordinal
string comparison will be used to determine reuse of named arguments.IDictionary<string, object>
overload will use itsTryGetValue
method for retrieving the value.FormatException
is thrown when a value for a named argument cannot be found.Updates
valueProvider
from aFunc<string, object>
to aFunc<string, (bool found, object value)>
to support specifying if the named argument was found.logFormat
value from configuration.