Skip to content
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

Visibility of Result<T>.Backtrack #88

Open
ewoutkramer opened this issue Mar 27, 2019 · 9 comments
Open

Visibility of Result<T>.Backtrack #88

ewoutkramer opened this issue Mar 27, 2019 · 9 comments

Comments

@ewoutkramer
Copy link

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
Copy link
Author

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
Copy link
Author

..and this is what I needed in the end: ewoutkramer@9c2674f

@nblumhardt
Copy link
Member

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

@AndrewSav
Copy link
Contributor

AndrewSav commented Aug 14, 2020

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
Copy link
Member

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
Copy link
Contributor

AndrewSav commented Apr 13, 2021

@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
Copy link
Author

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
Copy link
Contributor

AndrewSav commented Apr 13, 2021

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
Copy link
Member

👍 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.

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

Successfully merging a pull request may close this issue.

3 participants