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

Expand ParseResult API #780

Closed
martijnhoekstra opened this Issue Apr 14, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@martijnhoekstra

martijnhoekstra commented Apr 14, 2017

This is intended as a discussion issue.

Some more combinators and utility methods on ParseResult to make it easier to chain and to work with would be helpful to me. I would primarily want the LINQ API and failure recovery combinators.

The LINQ API would allow using parse results in LINQ statements. For example something like

from localdate in datepattern.Parse(datestring)
from localtime in timepattern.Parse(timestring)
select localdate + localtime

yielding a ParseResult<LocalDateTime>

Filtering with a where clause would also be possible

from localdate in datepattern.Parse(datestring)
from localtime in timepatter.Parse(timestring)
where localtime > LocalTime.Noon && localdate.DayOfWeek == IsoDayOfWeek.Wednesday
select localdate + localtime

giving a success if the parse is for a Wednesday afternoon, and a failure otherwise. The where filtering would result in some less elegant results as it would require a natural zero for ParseResult, which would require an Exception to be pulled from thin air. On first sight creating something like a FilterException for that could work.

The Select and SelectMany methods would also be useful in their own right. Select is already implemented (as Convert), but has an internal access restriction. Nevertheless, the fact that it already exists suggests it's useful at least to the authors of the library.

For recovery options, it would be nice to be able to fold or project the error case. Some example method signatures on ParseResult<T>:

U Fold<U>(Func<T, U> success, Func<Exception, U> failure) //could be implemented as Success ? success(Value) : failure(Exception)
T Recover(Func<Exception, T> recovery) //could be implemented as Fold(identity, recovery)
ParseResult<T> TryRecover(Func<Exception, ParseResult<T>> recovery) //could be implemented as Fold(t => ForValue(t), recovery))
T ValueOrElse(T fallback) //could be implemented as Recover(ignored => fallback)

Writing these kinds of combinators isn't currently possible for users of the library, since ParseResult is sealed, and it's not possible to construct a ParseResult manually, neither for the success of failure, since the factory

Is there interest in something like that? If there is, I wouldn't mind seeing if I can put that together in a working demo, so the merits of the thing can discussed with actual code behind it.

@jskeet

This comment has been minimized.

Member

jskeet commented Apr 14, 2017

I wouldn't want to add those methods to the existing API. I think the vast majority of users would find them confusing. However, I'd be willing to look at exposing just enough that they could be built in a separate package.

Would just exposing Convert be enough? Or possibly ForValue and ForException? (The nice thing about Convert is that it doesn't require the actual exception to be materialized - the exception provider is just passed along.)

@martijnhoekstra

This comment has been minimized.

martijnhoekstra commented Apr 14, 2017

Come to think about it, the LINQ syntax can be quite confusing for people who associate it only with collections, which is probably almost everyone.

I think ForValue and ForException would be sufficient to implement everything from the user side. Convert isn't sufficient to build a failure (at least, not without intentionally creating parse errors, which is a bridge to far even for me), which is needed for filter-like operations (where), and some utility in the chaining - without the ability to create a failure, there is much less utility in SelectMany. Additionally, without ForException there is no way to build an exception-trapping construct in to the construct. I'm not entirely sure whether that's a good idea in the first place though (food for thought).

@jskeet

This comment has been minimized.

Member

jskeet commented Apr 14, 2017

Right. Do you think you need the ContinueWithMultiple part? It's used in composite patterns mostly, IIRC.

@martijnhoekstra

This comment has been minimized.

martijnhoekstra commented Apr 14, 2017

I'm not entirely sure yet. I only quickly went through things, and it's not actually used within the class itself, other than passing it on. I guess it's used to indicate whether to propagate the first error or replace with the last, or accumulate under some error accumulating Exception, and I would want to be consistent with the core library over being principled probably - but for that I have to dive deeper in to how it's used exactly.

@jskeet

This comment has been minimized.

Member

jskeet commented Apr 14, 2017

Righto. Sounds like the best option is for you to explore and come up with a PR. I'm amenable to making those two methods public, possibly with an overload for ContinueWithMultiple. Aside from anything else, that's useful for anyone writing test code that needs a ParseResult.

@jskeet

This comment has been minimized.

Member

jskeet commented Apr 19, 2017

This was fixed by #788.

@jskeet jskeet closed this Apr 19, 2017

@malcolmr malcolmr added this to the 2.1.0 milestone Aug 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment