-
Notifications
You must be signed in to change notification settings - Fork 105
Added optional error message to the Where combinator #131
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
/// <returns>The resulting parser.</returns> | ||
public static TokenListParser<TKind, T> Where<TKind, T>(this TokenListParser<TKind, T> parser, Func<T, bool> predicate) | ||
public static TokenListParser<TKind, T> Where<TKind, T>(this TokenListParser<TKind, T> parser, Func<T, bool> predicate, string message = "unsatisfied condition") |
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.
If message
is nullable then the type should be string?
. Or if not, then an argument check is still required to catch misuse by consumers on earlier language versions. (Making it string?
seems reasonable to me, as it would match Result<T>.ErrorMessage
.)
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.
@nblumhardt In which cases does it make sense to have the message to be null
?
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.
🤔 - probably aren't any. string
and the null check would be fine 👍
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.
@nblumhardt added.
Would it be possible to tack a test on with this one? |
@nblumhardt the current |
Thanks for the reply! Originally, this codebase evolved pretty quickly, and test coverage is lower than it should be. I'm trying to bump it up a little with each change :-) How about: public class WhereCombinatorTests
{
[Fact]
public void WhereFailsIfPrecedingParserFails()
{
AssertParser.Fails(Character.EqualTo('a').Where(_ => true), "b");
}
[Fact]
public void WhereSucceedsWhenPredicateMatches()
{
AssertParser.SucceedsWith(Character.EqualTo('a').Where(a => a == 'a'), "a", 'a');
}
[Fact]
public void WhereFailsWhenPredicateDoesNotMatch()
{
AssertParser.FailsWithMessage(
Character.EqualTo('a').Where(a => a != 'a', "character should be an A"),
"a",
"Syntax error (line 1, column 1): character should be an A");
}
} |
@nblumhardt thank you for this, I have added it. |
Great, thanks 👍 |
Resolves #117