-
Notifications
You must be signed in to change notification settings - Fork 53
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 Parser.caret #301
Add Parser.caret #301
Conversation
actually, inclusive span could be bad because that means you can never use it with Parser0 which may parse nothing, so an inclusive range could fail there, but then again, maybe that is a feature? Maybe you only want the Span of a Parser? |
Codecov Report
@@ Coverage Diff @@
## main #301 +/- ##
==========================================
+ Coverage 96.43% 96.65% +0.21%
==========================================
Files 8 9 +1
Lines 1011 1404 +393
Branches 81 133 +52
==========================================
+ Hits 975 1357 +382
- Misses 36 47 +11
Continue to review full report at Codecov.
|
} | ||
} | ||
|
||
def toCaret(offset: Int): Option[Caret] = | ||
if (offset < 0 || offset > input.length) None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎨 this condition is popping a few times would a isValidOffset
be worth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea. I've made that change (and avoid checking it so many times).
This does the caret part of #238
related to #234
I did not add Span, just Caret.
My concern was that the span methods are a bit less clear. For instance, we may want an inclusive upper bound range, or maybe we do want exclusive. Inclusive is a bit more complex to implement however.
To sidestep this concern for now while we think about the design, I just added getting the
Caret
itself.cc @re-xyr