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

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

Closed

Conversation

ewoutkramer
Copy link

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

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

ewoutkramer commented Jul 4, 2019

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:

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

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

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

(and no is an acceptable answer BTW).

@nblumhardt
Copy link
Member

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!

@nblumhardt nblumhardt closed this Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visibility of Result<T>.Backtrack
2 participants