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

[API Proposal]: String should implement IParsable<string> #78842

Closed
Jcparkyn opened this issue Nov 25, 2022 · 8 comments · Fixed by #82836
Closed

[API Proposal]: String should implement IParsable<string> #78842

Jcparkyn opened this issue Nov 25, 2022 · 8 comments · Fixed by #82836
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime

Comments

@Jcparkyn
Copy link

Jcparkyn commented Nov 25, 2022

Background and motivation

The IParsable<T> interface was added in .NET 7 to support using generic types that can be parsed from a string (e.g., int and double). However, this was not extended to include string, which presents some consistency issues.

My specific use-case for this is a generic class for reading and writing query string parameters in Blazor WASM (see below). I'd like to have a single generic class which can be used as e.g. QueryParam<int> or QueryParam<string>, but the fact the string doesn't implement IParsable makes this impossible. Instead, I have to make a separate, non-generic class for handling strings, even though they should be the "easiest" case. I imagine that this issue would extend to many other use-cases for reading values from strings.

The class I'd like to be able to use (simplified):

public sealed class QueryParam<T> where T : ISpanParsable<T>, IFormattable
{
    private T _value;
    private readonly NavigationManager _navManager;
    private readonly string _name;

    public QueryParam(NavigationManager navManager, string name, T defaultValue)
    {
        _navManager = navManager;
        _name = name;
        var uri = navManager.ToAbsoluteUri(navManager.Uri);

        _value = QueryHelpers.ParseQuery(uri.Query).TryGetValue(name, out var val)
            && T.TryParse(val.ToString(), CultureInfo.InvariantCulture, out var parsed)
                ? parsed
                : defaultValue;
    }

    public T Value
    {
        get => _value;
        set
        {
            _value = value;
            var stringValue = value.ToString(null, CultureInfo.InvariantCulture);
            _navManager.NavigateTo(_navManager.GetUriWithQueryParameter(_name, stringValue), replace: true);
        }
    }
}

Then using it:

var name = new QueryParam<string>(_navManager, "name", "");
var age = new QueryParam<int>(_navManager, "age", 0);

Note: The above class would also require string to implement IFormattable in a similar fashion. However, much of the benefit could be had with just IParsable.

API Proposal

namespace System;

// These implementations use explicit interface implementation, so that the methods
// are not directly visible on string.
public class String : IParsable<String>
{
    // returns s
    static string IParsable<String>.Parse(string s);
    // returns true if s is non-null, and sets result = s.
    static bool IParsable<String>.TryParse(string? s, IFormatProvider? provider, out string result);
}

API Usage

A simpler (and less useful) example than the one above:

public static IEnumerable<T> Separate<T>(string source, char separator) where T : IParsable<T>
{
    return source.Split(separator)
        .Select(x => T.Parse(x, CultureInfo.InvariantCulture));
}

var ints = Separate<int>("11,12,13", ',');
var strings = Separate<string>("aa,bb,cc", ',');

Alternative Designs

It may be preferable to implement ISpanParsable<string> instead, for the same consistency reasons as above.

Risks

The obvious issue here is that these methods don't make much sense in isolation, so users may be confused by the presence of a string.Parse method.

@Jcparkyn Jcparkyn added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 25, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 25, 2022
@ghost
Copy link

ghost commented Nov 25, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

The IParsable<T> interface was added in .NET 7 to support using generic types that can be parsed from a string (e.g., int and double). However, this was not extended to include string, which presents some consistency issues.

My specific use-case for this is a generic class for reading and writing query string parameters in Blazor WASM (see below). I'd like to have a single generic class which can be used as e.g. QueryParam<int> or QueryParam<string>, but the fact the string doesn't implement IParsable makes this impossible. Instead, I have to make a separate, non-generic class for handling strings, even though they should be the "easiest" case. I imagine that this issue would extend to many other use-cases for reading values from strings.

The class I'd like to be able to use (simplified):

public sealed class QueryParam<T> where T : ISpanParsable<T>, IFormattable
{
    private T _value;
    private readonly NavigationManager _navManager;
    private readonly string _name;

    public QueryParam(NavigationManager navManager, string name, T defaultValue)
    {
        _navManager = navManager;
        _name = name;
        var uri = navManager.ToAbsoluteUri(navManager.Uri);

        _value = QueryHelpers.ParseQuery(uri.Query).TryGetValue(name, out var val)
            && T.TryParse(val.ToString(), CultureInfo.InvariantCulture, out var parsed)
                ? parsed
                : defaultValue;
    }

    public T Value
    {
        get => _value;
        set
        {
            _value = value;
            var stringValue = value.ToString(null, CultureInfo.InvariantCulture);
            _navManager.NavigateTo(_navManager.GetUriWithQueryParameter(_name, stringValue), replace: true);
        }
    }
}

Then using it:

var name = new QueryParam<string>(_navManager, "name", "");
var age = new QueryParam<int>(_navManager, "age", 0);

Note: The above class would also require string to implement IFormattable in a similar fashion. However, much of the benefit could be had with just IParsable.

API Proposal

namespace System;

// These implementations use explicit interface implementation, so that the methods
// are not directly visible on string.
public class String : IParsable<String>
{
    // returns s
    static string IParsable<String>.Parse(string s);
    // returns true if s is non-null, and sets result = s.
    static bool IParsable<String>.TryParse(string? s, IFormatProvider? provider, out string result);
}

API Usage

A simpler (and less useful) example than the one above:

public static IEnumerable<T> Separate<T>(string source, char separator) where T : IParsable<T>
{
    return source.Split(separator)
        .Select(x => T.Parse(x, CultureInfo.InvariantCulture));
}

var ints = Separate<int>("11,12,13", ',');
var strings = Separate<string>("aa,bb,cc", ',');

Alternative Designs

It may be preferable to implement ISpanParsable<string> instead, for the same consistency reasons as above.

Risks

The obvious issue here is that these methods don't make much sense in isolation, so users may be confused by the presence of a string.Parse method.

Author: Jcparkyn
Assignees: -
Labels:

api-suggestion, area-System.Runtime, untriaged

Milestone: -

@En3Tho
Copy link
Contributor

En3Tho commented Nov 25, 2022

I guess you might be interested in Roles/Shapes. Not sure if they will land in C# 12 though.

I think your best option is just to introduce some kind of NonEmptyString struct which will implement IParsable with more or less obvious mechanics.

With raw string as stated there is no sense having this from runtime point of view.

@tannergooding
Copy link
Member

CC. @jeffhandley, you had raised the same premise to me

@jeffhandley
Copy link
Member

I had nearly the exact same scenario a couple weeks ago while working on a command-line app where I needed to handle command-line parameters in the same way shown here. I came to the same conclusion that having String implement IParsable<string>, ISpanParsable<string>, IFormattable<string>, and ISpanFormattable<string> would have make parameter handling much cleaner. Parsing and formatting would be no-ops and would probably only be provided as explicit interface implementations.

(Thanks for the CC @tannergooding)

@dakersnar
Copy link
Contributor

The obvious issue here is that these methods don't make much sense in isolation, so users may be confused by the presence of a string.Parse method.

I think if these are explicitly implemented (as you outlined) this shouldn't be a huge problem. Personally, it makes sense that a String would be both parsable and formattable, even with the understanding that those operations are basically no-ops.

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Jan 31, 2023
@bartonjs
Copy link
Member

Going all the way to ISpanParsable seems legit.

Since the longest valid ReadOnlySpan<char> is longer than the longest valid string, the TryParse on a ROS-char should return false for a too-long input.

namespace System;

// These implementations use explicit interface implementation, so that the methods
// are not directly visible on string.
public partial class String : ISpanParsable<String>
{
    static string IParsable<String>.Parse(string s, IFormatProvider? provider) => s;
    // returns true if s is non-null, and sets result = s.
    static bool IParsable<String>.TryParse(string? s, IFormatProvider? provider, out string result);

    // also explicitly implement ISpanParsable
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 28, 2023
@tannergooding tannergooding self-assigned this Feb 28, 2023
@tannergooding
Copy link
Member

Will have a PR up covering this and #78523 shortly

@ghost ghost locked as resolved and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants