Add constructor for Result<> to ease writing custom tokenizers

datalust / superpower

A C# parser construction toolkit with high-quality error reporting

Apache License 2.0

1.07k stars 99 forks source link

Add constructor for Result<> to ease writing custom tokenizers #97

Closed ewoutkramer closed 1 month ago

ewoutkramer commented 5 years ago

This is a PR to fix #88.

It is likely that custom tokenizers need to set Result.Backtrack, this PR adds a constructor that makes it possible to set this property when creating a new Result.

nblumhardt commented 5 years ago

Thanks for this Ewout.

I was wondering - in the TokenizerBuilder case, I can't see where we'd actually make use of the backtracking flag - I think it's just being propagated there for the sake of consistency. Does your tokenizer need it, or can you pass false?

I'm guessing you've spent much more time thinking about this than I have :-) ... is there any way you can show the scenario in more detail?

ewoutkramer commented 5 years ago

The scenario was that I wanted to build my own tokenizer, so I started with a copy of your (internal) SimpleLinearTokenizer, since it's pretty representative of what I wanted.

However, I got compiler errors for two reasons:

Presentation.FormatExpectation is not public - which I think is unfortunate, since I want the errors produced by my custom tokenizer to be as close as the built-in ones as possible.
It calls a constructor for Result<T> here (https://github.com/datalust/superpower/blob/dev/src/Superpower/Tokenizers/TokenizerBuilder.cs#L187) that is internal.

I think it would be good that a basic tokenizer as the SimpleLinearTokenizer can be used as a starting point - and thus it should only use public methods.

nblumhardt commented 5 years ago

Interesting - makes sense! Thanks for the details. I agree that being able to base code off of the internal implementations would be worthwhile 👍

I need some time to wrap my head around different options for the API, here - I'll try to loop back to this as soon as I can.

ewoutkramer commented 10 months ago

Hi Nicholas - is there any chance you'll be looking into this? I worked around it, so it didn't bother me enough anymore to keep pushing. But it is still on my todo lists, so I wondered, shall I take it off? I can totally understand why this isn't really hi-prio, being an open source repo maintainer myself ;-)

ewoutkramer commented 10 months ago

(and no is an acceptable answer BTW).

nblumhardt commented 10 months ago

Hi @ewoutkramer! Alas, I haven't had any time to loop back here. I'm working in a fairly distant part of the stack at the moment, so getting back up to speed on all things Superpower is a bit tricky. If you've worked around this successfully it might be best dropped from the TODO list for now, although it's still good to have on my radar. Thanks for checking in!