Add DefaultParser and DefaultParser0 typeclasses #577
Conversation
These provide a way to bless canonical `Parser`/`Parser0` instances and easily call them.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #577 +/- ##
==========================================
- Coverage 95.13% 94.68% -0.45%
==========================================
Files 10 12 +2
Lines 1480 1487 +7
Branches 330 332 +2
==========================================
Hits 1408 1408
- Misses 72 79 +7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This seems pretty good to me. We need to fix the header check (which I find a bit annoying but sbt-typelevel sets it up).
I also wonder a bit about this typeclass being lawless.
It could be a lawful typeclass if paired with a function that converts A to String and the law is that we can always parse and get back the original.
What do you think about the lawlessness?
| } | ||
|
|
||
| implicit def defaultParserIsDefaultParser0[A: DefaultParser]: DefaultParser0[A] = | ||
| DefaultParser0.instance(DefaultParser[A].parser) |
There was a problem hiding this comment.
We don't need DefaultParser0 here do we?
There was a problem hiding this comment.
Not particularly, I'll occasionally include the object for methods that are kind of generic.
I don't have strong feelings about it, and am happy to remove it if you prefer
I don't have any strong opinions about it being lawless. I have the mild inclination that creating laws for It'd probably make testing parsers over the set of valid inputs easier, so it might be worth doing for that reason alone. |
Actually used `sbt headerCreateAll` this time, like I should have the first time
| * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
| */ | ||
|
|
||
| package cats.parse.extra |
There was a problem hiding this comment.
I think we should put this in the main package. There are very few names in cats.parse already.
| * @tparam A | ||
| */ | ||
| trait DefaultParser0[+A] { | ||
| def parser0: Parser0[A] |
There was a problem hiding this comment.
Can we add plumbing through parse and parseAll here so you can do: DefaultParser[Foo].parseAll(s)
There was a problem hiding this comment.
Easy enough 👍🏻
There was a problem hiding this comment.
I don't see this change yet. Did you reconsider this?
There was a problem hiding this comment.
Looks like it's up, my guess is it took a couple minutes to propagate to the UI
|
Thanks for everyone's help :) |
These provide a way to bless canonical
Parser/Parser0instances and easily call them.Implements #435