datalust / superpower

A C# parser construction toolkit with high-quality error reporting
Apache License 2.0
1.07k stars 99 forks source link

Visibility of Result<T>.Backtrack #88

Open ewoutkramer opened 5 years ago

ewoutkramer commented 5 years ago

I have tried to build an alternative tokenizer, based on the TokenizerBuilder in this repo. I hit a problem where the code for TokenizerBuilder uses Result.Backtrack - this is an internal property. Which makes me wonder: if the current SimpleLinearTokenizer needs access to this property, wouldn't other custom tokenizers need it too?

ewoutkramer commented 5 years ago

To be more precise, this is the problematic part:

 failure = new Result<TKind>()(remainder, augmentedMessage, attempt.Expectations, attempt.Backtrack);

It's not unlikely that custom tokenizer would need to augment the error message and position, just like the above statement.

ewoutkramer commented 5 years ago

..and this is what I needed in the end: https://github.com/ewoutkramer/superpower/commit/9c2674f28fec0dae575f117a5f6c6a0f856e6a3e

nblumhardt commented 5 years ago

Thanks @ewoutkramer - those look like reasonable modifications, I'll take a look at this.

AndrewSav commented 4 years ago

I have a different use case that needs Backtrack.

I want to write another combinator, similar to Many. This combinator would accept a lower and an upper bound, similar to regex {n,m}. I want my combinator to conform Many style but I cannot, since Many has access to Backtrack and I don't. @ewoutkramer solution unfortunately will not help that use case.

nblumhardt commented 3 years ago

Looking at this again, I wonder if we should consider just making all the internal properties and constructors on Result<T> (and TokenListParserResult<T,U>) public 🤔

I originally steered away from this because I think they're pretty tightly bound to the internal implementation, but in retrospect, making experimentation and add-on development easier seems like a reasonable trade-off.

AndrewSav commented 3 years ago

@nblumhardt As a user I support this, because it would allow me to do what I want. I'm not sure if I would be comfortable with it as a designer, but fortunately, I'm not the designer, you are. ;) So I'll leave it up to you.

ewoutkramer commented 3 years ago

As a fellow API designer we all know that you'll be stuck with your public interface for a long time. So, I'd do it just "on demand".

AndrewSav commented 3 years ago

It probably does not make sense to open just a single property, if for nothing else, then for consistency reasons. They were designed as a part of an internal "interface" (I do not mean C# interface here, I mean it in more general sense) that we are considering making public here. If we are comfortable with this, it certainly makes sense. I cannot think of better options right now. I also agree, that it helps with experimentation and the longer term library enhancements.

nblumhardt commented 3 years ago

👍 both angles are definitely valid. The current shape of these APIs has been stable for quite a long time now, so I'm leaning more towards favouring completeness and consistency in this case, and exposing all of the interface.