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 Parser.caret #301

Merged
merged 4 commits into from
Nov 12, 2021
Merged

Add Parser.caret #301

merged 4 commits into from
Nov 12, 2021

Conversation

johnynek
Copy link
Collaborator

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

@johnynek
Copy link
Collaborator Author

johnynek commented Nov 11, 2021

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

codecov-commenter commented Nov 11, 2021

Codecov Report

Merging #301 (bcff7b1) into main (714a9df) will increase coverage by 0.21%.
The diff coverage is 97.22%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
core/shared/src/main/scala/cats/parse/Parser.scala 96.47% <90.00%> (+0.15%) ⬆️
core/shared/src/main/scala/cats/parse/Caret.scala 100.00% <100.00%> (ø)
...shared/src/main/scala/cats/parse/LocationMap.scala 100.00% <100.00%> (+2.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 714a9df...bcff7b1. Read the comment docs.

}
}

def toCaret(offset: Int): Option[Caret] =
if (offset < 0 || offset > input.length) None
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

@johnynek johnynek merged commit 22b34f1 into main Nov 12, 2021
@johnynek johnynek deleted the oscar/add_caret branch November 12, 2021 17:56
@johnynek johnynek mentioned this pull request Mar 28, 2022
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.

3 participants