Adding overloads to Document for GetValues and Get

[Semirk0]

Adding overload for GetValues() and Get() to support in Document. GetStringValue() method in now only being called once.

[NightOwl888]

Thanks for the PR. Great work! This is looking good so far, but I have requested a few changes before this can be merged.

While it wasn't part of the original scope of #362, after seeing the impact of this it occurred to me that the user experience can be improved by doing the following:

1. Remove ToString(IFormatProvider provider) from IIndexableField, as this is a leaky abstraction. The fact this forces ToString(IFormatProvider) into the tests is proof of this. Instead, we should allow IIndexableField implementations to opt in if they want to support formatting by implementing IFormattable. Don't inherit IFormattable from IIndexableField, allow them to be used independently.
2. We technically only need 1 implementation of ToString on each IIndexableField type, the rest of the overloads can just cascade their call. See this example in J2N.
3. Change logic of formattable Field.ToString() methods to test for if (field is IFormattable formattable) and then call the formattable.ToString(string, IFormatProvider) overload. The reasonable default values of the parameters are null and J2N.Text.StringFormatter.CurrentCulture for those overloads that don't include both. In the case null is passed, just use the original string format (in the case of field it was {0}<{1}:{2}>). If the formattable check doesn't pass, simply call the default field.ToString() overload.
4. Implement IFormattable on the built-in Field and LazyField types. Also add the other 3 overloads ToString(), ToString(string format) and ToString(IFormatProvider provider) so each built-in IIndexableField type has all 4 overloads.
5. Change the Document class to be similar, implementing IFormattable on the built-in types but allowing implementers to opt out of formatting. However, since a format string doesn't make much sense in this case, explicitly implement IFormattable (which will hide it from the API of Document). Therefore, the Document class will still only have 2 public overloads ToString() and ToString(IFormatProvider) that will cascade their call to an explictly implmented ToString(string, IFormatProvider) method.

If you analyze the source of StringBuilder.Append, it shows exactly what is expected of how to support IFormattable.

I think we should consider supporting ICustomFormatter too, but we need to ponder exactly how it should be done. No need to make it part of this PR.

[Semirk0]

Can you explain what you mean by

3. Change logic of formattable Field.ToString() methods to test for if (field is IFormattable formattable)

Do you mean to change logic in class Field or Document?

Also, I don't really understand 5. either. Especially

Therefore, the Document class will still only have 2 public overloads ToString() and ToString(IFormatProvider) that will cascade their call to an explictly implmented ToString(string, IFormatProvider) method.

Can you elaborate?

[NightOwl888]

I guess it is simpler just to show the code than to describe it.

We test for IFormattable in the if statement to make it an optional feature of implementations of either IIndexableField or FieldsValue. If the type implements IFormattable, we call IFormattable.ToString(string, IFormatProvider). If not, we just skip formatting and call ToString() on the object. This makes it possible to use an IIndexableField implementation that has no ToString() implementations on it or to implement both IIndexableField and IFormattable to provide localizable formatting support.

In all implementations of IIndexableField (4 overloads of ToString())

public override string ToString()
{
    return ToString(null, J2N.Text.StringFormatter.CurrentCulture);
}

public virtual string ToString(string format)
{
    return ToString(format, J2N.Text.StringFormatter.CurrentCulture);
}

public virtual string ToString(IFormatProvider provider)
{
    return ToString(null, provider);
}

public virtual string ToString(string format, IFormatProvider provider)
{
    StringBuilder result = new StringBuilder();
    result.Append(m_type.ToString());
    result.Append('<');
    result.Append(m_name);
    result.Append(':');

    if (FieldsData != null)
    {
        if (FieldsData is IFormattable formattable)
            result.Append(formattable.ToString(format, provider);
        else
            result.Append(FieldsData.ToString());
    }

    result.Append('>');
    return result.ToString();
}

In Document (2 public and one hidden overload of ToString())

public override string ToString()
{
    return ToString(null, J2N.Text.StringFormatter.CurrentCulture);
}

public virtual string ToString(IFormatProvider provider)
{
    return ToString(null, provider);
}

string IFormattable.ToString(string format, IFormatProvider provider) // <- explicit implementation
{
    var buffer = new StringBuilder();
    buffer.Append("Document<");
    for (int i = 0; i < fields.Count; i++)
    {
        IIndexableField field = fields[i];
        if (field is IFormattable formattable)
            buffer.Append(formattable.ToString(format, provider));
        else
            buffer.Append(field.ToString());
        if (i != fields.Count - 1)
        {
            buffer.Append(" ");
        }
    }
    buffer.Append(">");
    return buffer.ToString();
}

Be sure that all built-in IIndexableField types, the Field.Number type, and Document all implement IFormattable themselves.

[Semirk0]

Is everything ok with my last commit or should I change something?

[NightOwl888]

My apologies for not getting to this sooner. I started working on some tests for this (as per #362), but discovered a formatting bug in J2N and then got sidetracked. In Java, floating point numbers are always formatted with at least 1 decimal place, but in .NET the decimal is removed if it is a whole number. In J2N that 1 decimal place is sometimes being truncated.

I'll work on getting the bug fixed and completing the tests, but in the meantime there are a few changes that are requested from the last commit.

[NightOwl888]

I reverted the merge, since it caused a conflict. For future reference please rebase against master rather than merge it.

I also made some minor changes to close #362.