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

Bytes Reader #77

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Bytes Reader #77

wants to merge 6 commits into from

Conversation

lostinplace
Copy link
Contributor

Offering this up in case you want it. There are a couple of things here that I think are more generally valuable, but I wanted your opinion on them.

Also, I have no idea where the tests should go

added a drop method to sequence reader.  without it an exception is generated when dealing with byte sequences
* Made change to _options basic parser to allow for optional readers
* Added a `BytesReader` to the reader module to allow for reading bytes, which makes parsing binary files easier
* Made it so that readers inheriting `SequenceReader` can optionally override `drop` and `rest`
* Added tests, but I'm not sure where they should go
* Added a `rep_n` parser to the `repeated` module, which is like `rep` but takes a number of times to repeat.  This was already accomplishable using `min=x, max=x`, but this is a little more readable.

added byteparsers
@codecov-commenter
Copy link

Codecov Report

Merging #77 (7366361) into master (dba1b6e) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff            @@
##            master       #77   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines          836       890   +54     
  Branches       100       111   +11     
=========================================
+ Hits           836       890   +54     
Impacted Files Coverage Δ
src/parsita/options.py 100.00% <100.00%> (ø)
src/parsita/parsers/__init__.py 100.00% <100.00%> (ø)
src/parsita/parsers/_repeated.py 100.00% <100.00%> (ø)
src/parsita/state/__init__.py 100.00% <100.00%> (ø)
src/parsita/state/_reader.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@drhagen
Copy link
Owner

drhagen commented Jun 2, 2023

I used to think that we needed a different Reader for each context, but then your drop PR (#73) made me think that I had it wrong. Right now, the parse method is different depending on the context, but this PR is going to make me investigate if the parse method can be constant and the Reader be only a function of the input type, which would tremendously reduce the complexity.

This mirrors the complexity where lit means something totally different in each context. If you could comment on #60, that would be great. I don't actually use GeneralParsers anymore, so I am not sure of what users' expectations are.

I'm not sure how I feel about rep_n, but that's a relatively minor side issue to BytesReader. I'll play around with this and try to integrate it cleanly into Parsita.

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