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

Add string.RemovePrefix/RemoveSuffix (dupe) #27414

Closed
grant-d opened this issue Sep 17, 2018 · 6 comments
Closed

Add string.RemovePrefix/RemoveSuffix (dupe) #27414

grant-d opened this issue Sep 17, 2018 · 6 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime
Milestone

Comments

@grant-d
Copy link
Contributor

grant-d commented Sep 17, 2018

[Picking up an item from up-for-grabs]

It is useful to have methods that trim a specified prefix or suffix from a string. This is somewhat simple for a developer to write themselves, but it seems to be a common enough request that it would be useful to support directly, especially in regards to robustness and performance.

Here are some references gleaned from the original issue.
http://stackoverflow.com/questions/7170909/trim-string-from-end-of-string-in-net-why-is-this-missing
http://stackoverflow.com/questions/4101539/c-sharp-removing-strings-from-end-of-string
http://stackoverflow.com/questions/5284591/how-to-remove-a-suffix-from-end-of-string
http://stackoverflow.com/questions/4335878/c-sharp-trimstart-with-string-parameter

Usage

There are only 2 overloads for each method, as shown in the following examples:

// Default overload
"http://foo.com".TrimPrefix("http://").TrimSuffix(".com") == "foo"

// StringComparison
"http://foo.com".TrimPrefix("HTTP://", StringComparison.OrdinalIgnoreCase) == "foo.com"

Background

Proposed API

/// <summary>
/// Removes the specified prefix if it is found at the start of the string.
/// String comparison uses <see cref="StringComparison.Ordinal"/>.
/// </summary>
/// <param name="prefix">The prefix to remove.</param>
public string RemovePrefix(string value);

/// <summary>
/// Removes the specified prefix if it is found at the start of the string.
/// String comparison uses the specified <see cref="StringComparison"/>.
/// </summary>
/// <param name="suffix">The prefix to remove.</param>
/// <param name="comparisonType">The string comparison method to use.</param>
public string RemovePrefix(string value, StringComparison comparisonType);

/// <summary>
/// Removes the specified suffix if it is found at the end of the string.
/// String comparison uses <see cref="StringComparison.Ordinal"/>.
/// </summary>
/// <param name="suffix">The suffix to remove.</param>
public string RemoveSuffix(string value);

/// <summary>
/// Removes the specified suffix if it is found at the end of the string.
/// String comparison uses the specified <see cref="StringComparison"/>.
/// </summary>
/// <param name="suffix">The suffix to remove.</param>
/// <param name="comparisonType">The string comparison method to use.</param>
public string RemoveSuffix(string value, StringComparison comparisonType);

Details, Decisions, Questions

Some of these items from @tarekgh's feedback: https://github.com/dotnet/corefx/issues/1244#issuecomment-261606399

  • Decision: namespace System
    Decision: No bool repeat overloads. The callsite can call this recursively (at the risk of more allocations).
    • Looking that linked StackOverflow questions it seems folks want the non-repeating behavior, i.e. "SIdId".RemoveSuffix("Id") should return "SId", not "S".
    • We don't want to introduce overloads for TrimEnd and TrimStart that have a non-repeating behavior because it's inconsistent with the existing ones.
    • At the same time, we feel the bool option is overkill and not very readable from the call site.
  • Should be instance methods on String (as opposed to extension methods)
  • Should we add corresponding methods to TextInfo (or whatever they care called in globalization)?

POC

This is a relatively simple addition, POC code below:

/// <summary>
/// Removes the specied prefix if it is found at the start of the string.
/// String comparison uses <see cref="StringComparison.Ordinal"/>.
/// </summary>
/// <param name="suffix">The prefix to remove.</param>
public string RemovePrefix(string prefix, StringComparison comparisonType)
{
    if (prefix == null)
        throw new ArgumentNullException(nameof(prefix));

    if (prefix.Length == 0)
        return this;

    int len = this.Length - prefix.Length;
    if (len < 0)
        return this;

    var found = StartsWith(prefix, comparisonType);
    if (!found)
        return this;

    if (len == 0)
        return string.Empty;

    var val = InternalSubString(prefix.Length, len);
    return val;
}

/// <summary>
/// Removes the specied suffix if it is found at the end of the string.
/// String comparison uses <see cref="StringComparison.Ordinal"/>.
/// </summary>
/// <param name="suffix">The suffix to remove.</param>
public string RemoveSuffix(string suffix)
    => RemoveSuffix(suffix, StringComparison.Ordinal);

/// <summary>
/// Removes the specied suffix if it is found at the end of the string.
/// String comparison uses the specified <see cref="StringComparison"/>.
/// </summary>
/// <param name="suffix">The suffix to remove.</param>
/// <param name="comparisonType">The string comparison method to use.</param>
public string RemoveSuffix(string suffix, StringComparison comparisonType)
{
    if (suffix == null)
        throw new ArgumentNullException(nameof(suffix));

    if (suffix.Length == 0)
        return this;

    int len = this.Length - suffix.Length;
    if (len < 0)
        return this;

    var found = EndsWith(suffix, comparisonType);
    if (!found)
        return this;

    if (len == 0)
        return string.Empty;

    var val = InternalSubString(0, len);
    return val;
}
@karelz
Copy link
Member

karelz commented Sep 17, 2018

Why did we split it off the original issue #14386?

@grant-d
Copy link
Contributor Author

grant-d commented Sep 17, 2018

[Edited] Because of the ask for a formal proposal. My understanding of the "example of a good proposal" was that the formal spec should be in the preamble, and not require scrolling through discussion.

I can inline it here if that's preferable for easier tracking?

@danmoseley
Copy link
Member

It's unfortunate this allocates a temporary string: "http://foo.com".TrimPrefix("http://").TrimSuffix(".com") but perhaps it's not common to do both.

Since this original issue was discussed, Span<char> has become available. Should these be added to the extension methods:
https://github.com/dotnet/corefx/blob/master/src/System.Memory/ref/System.Memory.cs#L85

@tarekgh
Copy link
Member

tarekgh commented Sep 17, 2018

Because of the ask for a formal proposal. I can inline it here if that's preferable?

Can't we just update the original proposal? now we have 2 issues tracking the same thing.

@grant-d
Copy link
Contributor Author

grant-d commented Sep 17, 2018

I just added Span.Contains in a separate PR, so happy to follow suit with these methods.

@grant-d
Copy link
Contributor Author

grant-d commented Sep 17, 2018

OK, updated original issue: https://github.com/dotnet/corefx/issues/1244

@grant-d grant-d closed this as completed Sep 17, 2018
@grant-d grant-d changed the title Add string.RemovePrefix/RemoveSuffix (draft) Add string.RemovePrefix/RemoveSuffix (dupe) Sep 17, 2018
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime
Projects
None yet
Development

No branches or pull requests

5 participants