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

Feature request: byte offset in conduitParser #331

Closed
beojan opened this issue Oct 17, 2017 · 4 comments
Closed

Feature request: byte offset in conduitParser #331

beojan opened this issue Oct 17, 2017 · 4 comments

Comments

@beojan
Copy link
Contributor

@beojan beojan commented Oct 17, 2017

It would be rather nice if conduitParser also returned a byte-offset instead of just line and column number. With very large files, the byte-offset would let you seek to the appropriate position instead of having to read the file to that point to skip lines.

@beojan

This comment has been minimized.

Copy link
Contributor Author

@beojan beojan commented Oct 18, 2017

I used conduit-tokenize-attoparsec instead, since that provides this functionality. It may be a good implementation to include in conduit-extra, though it doesn't allow tracking both byte offset and line / column position.

EDIT: Remove irrelevant question.

snoyberg added a commit that referenced this issue Oct 19, 2017
@snoyberg

This comment has been minimized.

Copy link
Owner

@snoyberg snoyberg commented Oct 19, 2017

Even though the change is a breaking change (which I normally avoid like the plague), it's a minor change. I've pushed a new branch (331-offset). The tests aren't passing yet, and I'm about 95% certain I've got off-by-one errors in there. Unfortunately other things have got my attention for the next two weeks, but if you're able to get the tests passing and send a PR, I'd be OK with the change.

@beojan

This comment has been minimized.

Copy link
Contributor Author

@beojan beojan commented Oct 19, 2017

Done. Turns out it's the tests that needed to be fixed.

@snoyberg

This comment has been minimized.

Copy link
Owner

@snoyberg snoyberg commented Oct 20, 2017

Fixed with #332.

@snoyberg snoyberg closed this Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.