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

Line number shouldn't increment until after \n #34

Closed
wavebeem opened this issue Dec 4, 2021 · 4 comments
Closed

Line number shouldn't increment until after \n #34

wavebeem opened this issue Dec 4, 2021 · 4 comments

Comments

@wavebeem
Copy link
Owner

wavebeem commented Dec 4, 2021

  • \n should probably be considered as part of the current line. Only the next character should advance the line number.

jneen/parsimmon#331

@hillin
Copy link

hillin commented Dec 8, 2021

\n should probably be considered as part of the current line. Only the next character should advance the line number.

I'm expecting this to happen!

.node should be inclusive of the end, not exclusive. Will need some new kind of combinator like lastIndex, if possible

Just to reiterate what was said in the issue referenced above, I still don't think inclusive range is a good idea (almost all Range implementations have exclusive end index).

@wavebeem
Copy link
Owner Author

wavebeem commented Dec 9, 2021

If you want to work on a PR feel free. I haven't been actively developing bnb for a while, but it is 0.x so I can release breaking changes if need be lol

@wavebeem wavebeem changed the title Inclusive ranges Line number shouldn't increment until after \n Dec 9, 2021
hillin added a commit to hillin/bread-n-butter that referenced this issue Dec 16, 2021
@hillin
Copy link

hillin commented Dec 16, 2021

I tried to create a PR but then I realized bnb does not have the same behavior as Parsimmon. bnb handles \n exactly as I wanted, line number does not increment until after it.

The same old example:

// parsimmon:
const parser = P.seqMap(P.index, P.digits, P.index, function (start, value, end) {
  console.log(start, value, end);
  return value;
});

parser.parse('1234\n');
// => {offset: 0, line: 1, column: 1} '1234' {offset: 4, line: 2, column: 0}

// bnb:
const parser = bnb.all(bnb.match(/\d+/).node('num'), bnb.text('\n').node('newline'));
console.log(parser.parse('1234\n').value[0]);
// =>{"type":"ParseNode","name":"num","value":"1234","start":{"index":0,"line":1,"column":1},"end":{"index":4,"line":1,"column":5}}

So apparently this issue does not exist in bnb at all.

@wavebeem
Copy link
Owner Author

Turns out I don't even remember how my own library works lol. Well that's great! I suppose I'll close the issue then.

Internal bnb code to handle line/col info:
https://github.com/wavebeem/bread-n-butter/blob/main/src/bread-n-butter.ts#L455-L472

image

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

No branches or pull requests

2 participants