Fix index out-of-range exception when deleting script files #748
Conversation
rjmholt
requested review from
SeeminglyScience,
TylerLeonhardt and
rkeithhill
| /// <param name="text">Input string to be split up into lines.</param> | ||
| /// <returns>The lines in the string.</returns> | ||
| public static IList<string> GetLines(string text) | ||
| public static List<string> GetLines(string text) |
There was a problem hiding this comment.
I realise this is a breaking change, but I without RemoveRange this gets trickier, and more importantly, less performant.
There was a problem hiding this comment.
Oof that's an odd one. Kind of seems like that shouldn't have been public.
I think it's a pretty safe bet that no one is using it... I would have never thought to look for a static get lines method on that class. That said, you could make an alternate private GetLinesInternal that is typed as List<string> then keep this method the same and have it implicitly cast to IList<string>
Either way, might consider decorating with Obsolete. That should probably either be moved to a utility class or just made private.
There was a problem hiding this comment.
I get the impression that there are quite a few APIs exposed as public that really should be internal PSES, which is something I would ideally like to address with v2. It's a bit tricky because the way the project is broken up means some things need to be shared between libraries, but on the other hand those things are opportunities to design a real module API. But yeah, a lot of our public APIs I think need to be retired or changed.
| /// <param name="line">The 1-based line to be validated.</param> | ||
| /// <param name="column">The 1-based column to be validated.</param> | ||
| /// <param name="isInsertion">If true, the position to validate is for an applied change.</param> | ||
| public void ValidatePosition(int line, int column, bool isInsertion) |
There was a problem hiding this comment.
Similarly, this is also a breaking change -- so maybe we ought to keep it in? I'd prefer not personally, but the preference isn't that strong.
There was a problem hiding this comment.
Maybe have something like this?
public void ValidatePosition(int line, int column)
{
// Actual logic
}
[Obsolete("Use ValidatePosition(int, int) instead")]
public void ValidatePosition(int line, int column, bool isInsertion)
{
ValidatePosition(line, column);
}I think it's worth avoiding the breaking change here, even if the risk is low.
rkeithhill
left a comment
rkeithhill
left a comment
There was a problem hiding this comment.
I like this better. LGTM
4 participants
Fixes #739.