Adding more StringSegment functionality - #230
Conversation
| } | ||
|
|
||
| // This handles StringSegment.Equals(string, StringSegment, StringComparison) and StringSegment.Equals(StringSegment, string, StringComparison) | ||
| // vai the implicit type converter |
There was a problem hiding this comment.
Couldn't we just add an instance method Equals(string) to this? The implicit operator has marginal utility outside of this given that we use var typing everywhere.
There was a problem hiding this comment.
The implicit operator has been very useful in HttpAbstractions. I can convert all of the constructor and method parameters on my types to StringValues without breaking any callers or adding extra overloads.
StringSegment is easier to use when it can provide one-to-one replacements for existing string methods. Yes I could rewrite those call sites to .Equals(string), but this is quicker and cleaner.
| } | ||
|
|
||
| // This handles StringSegment.Equals(string, StringSegment, StringComparison) and StringSegment.Equals(StringSegment, string, StringComparison) | ||
| // vai the implicit type converter |
There was a problem hiding this comment.
Couldn't we just add an instance method Equals(string) to this? The implicit operator has marginal utility outside of this given that we use var typing everywhere.
| { | ||
| /// <summary> | ||
| /// A <see cref="StringSegment"/> for a null <see cref="string"/>. | ||
| /// </summary> |
There was a problem hiding this comment.
Yea, this is a strange one. Why do you need none?
There was a problem hiding this comment.
It was more useful before I added the implicit string conversion. Removed.
|
/cc @KrzysztofCwalina it's growing 😄 |
| { | ||
| throw new IndexOutOfRangeException(); | ||
| } | ||
| if (index - Offset >= Length) |
There was a problem hiding this comment.
This should match the offset checks in the ctor:
| return !left.Equals(right); | ||
| } | ||
|
|
||
| // Do NOT add a implicit converter from StringSegment to String. That would negate most of the perf safety. |
| /// <summary> | ||
| /// Creates a new <see cref="StringSegment"/> from the given <see cref="string"/>. | ||
| /// </summary> | ||
| /// <param name="value"></param> |
| /// <summary> | ||
| /// Retrieves a <see cref="StringSegment"/> that represents a substring from this <see cref="StringSegment"/>. | ||
| /// The <see cref="StringSegment"/> starts at the position specified by <paramref name="offset"/>. | ||
| /// </summary> |
There was a problem hiding this comment.
A substring of this instance of StringSegment that begins at offset? Doesn't sound right as it is now
ffc8148 to
6881708
Compare
ghost
locked as resolved and limited conversation to collaborators
5 participants
#213 Needed for aspnet/HttpAbstractions#758