-
Notifications
You must be signed in to change notification settings - Fork 109
Add index/range support for TextSpan #163
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
Conversation
|
Thanks for the PR! Are there any places in the Superpower codebase, or typical usage patterns, where slicing is useful with |
Well...
Primarily, it should be supported because other similar types* support slicing, such as One benefit to meeting the slicing "interface" (it's duck-typed, not a true interface), aside from using the range operator ( My specific use case: I "reached for" the non-existant slice capability in a parser I was writing. This language has three different kinds of strings (unquoted, single-quoted, double-quoted). Those strings can be appended. Double quoted strings support escaping and trimming (the trimming is much like C#'s raw strings). I found it easiest to, in my parsing method, just accept the token values - quotes and all - and then pass the final result to another method that would handle concatenation, escaping, trimming, etc. That method would use the quotes to know how to handle that portion of the string. It would then strip the quotes off, with a simple * Notably, |
nblumhardt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the follow-up. I realise now that you haven't made the changes you mention because these would require a polyfill to keep the netstandard2.0 target happy; I think the choice to skip internal changes is the right one 👍
LGTM, just had one trivial nitpick
src/Superpower/Model/TextSpan.cs
Outdated
| return true; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - extra newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise now that you haven't made the changes you mention because these would require a polyfill to keep the netstandard2.0 target happy
Yeah. As it is, if they're using .NET Standard 2.0, then they'd need to use the polyfill to use the range operator (..).
If they're using .NET Core (not sure what the minimum version is), it just works, without polyfill package.
|
Thanks @binarycow 👍 |
This PR adds to
TextSpan, an indexer, and support for the range operator (..)Exactly how well this is supported depends on the target framework of the application that uses this library:
Slicemethods work as expected.