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

feat: Move non-grammar combinators to Parser #40

Merged
merged 27 commits into from
Dec 16, 2022
Merged

feat: Move non-grammar combinators to Parser #40

merged 27 commits into from
Dec 16, 2022

Conversation

epage
Copy link
Collaborator

@epage epage commented Dec 16, 2022

This is a followup to #37 and is working towards rust-bakery/nom#1408

The criteria for moving parsers was if they were common enough and did not affect the grammar. This way, the code structure can help highlight the grammar. This means I left out cut. While it mutates the error, like complerte, it does so in a way that affects parsing and it should be elevated (I think). I'm still a little mixed on that one.

Highlights

  • consumed -> Parser::with_recognized
    • The problem is recognize and consumed parallel each other
      but its not obvious by the name. combine calls it
      recognize_with_value. nom-supreme takes the approach of
      Parser::with_recognized. I feel like that makes the intent clear.
      The problem comes in that with_recognized implies an order
      (u32.with_recognized()), so I needed to swap the order of the tuple
      elements compared to consumed, combines recognize_with_value, and
      nom-supremes with_recognized.
  • dbg_dmp -> Parser::dbg_err
    • The name changed because we are only printing errors, we might want to
      print something else in the future. See that commit for more details
    • Support for any input that supports AsBytes, instead of just &[u8]
    • Allows any displayable context
    • Print to stderr, instead of stdout

So far, I've been using the combinator's name when moving them to
`Parser`.  The problem is `recognize` and `consumed` parallel each other
but its not obvious by the name.   `combine` calls it
`recognize_with_value`.  `nom-supreme` takes the approach of
`Parser::with_recognized`.  I feel like that makes the intent clear.
The problem comes in that `with_recognized` implies an order
(`u32.with_recognized()`), so I needed to swap the order of the tuple
elements compared to `consumed`, `combine`s `recognize_with_value`, and
` `nom-supreme`s `with_recognized`.
This is a translation of `dbg_dmp`.

The name changed because we are only printing errors, we might want to
print something else in the future

Random thoughts on the name
- `debug` is out because that implies a debug build (`debug_assert`)
- `dbg` mirrors `dbg!` for dumping content to output
- `_err` is a common suffix (`unwrap_err`)
- We want this easily discoverably to audit to prevent going into
  production.  People likely will audit for `dbg` so having a similar
  name will likely get this caught as well.

I had also considered `dump_err` which I favored until I considered code
audits.

As for functionality, this adds
- Support for any input that supports `AsBytes`, instead of just `&[u8]`
- Allows any displayable context
- Print to stderr, instead of stdout
@coveralls
Copy link

coveralls commented Dec 16, 2022

Pull Request Test Coverage Report for Build 3709890254

  • 64 of 90 (71.11%) changed or added relevant lines in 4 files are covered.
  • 19 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.03%) to 59.003%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 14 16 87.5%
src/combinator/mod.rs 43 55 78.18%
src/error.rs 6 18 33.33%
Files with Coverage Reduction New Missed Lines %
src/character/complete.rs 1 81.41%
src/lib.rs 2 0%
src/parser.rs 2 42.57%
src/error.rs 3 18.49%
src/combinator/mod.rs 11 65.11%
Totals Coverage Status
Change from base Build 3699347704: -0.03%
Covered Lines: 1822
Relevant Lines: 3088

💛 - Coveralls

@epage epage deleted the parser branch December 16, 2022 03:12
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.

2 participants