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!: Use 4 spaces for indentation. #390

Merged
merged 5 commits into from
Sep 11, 2024
Merged

feat!: Use 4 spaces for indentation. #390

merged 5 commits into from
Sep 11, 2024

Conversation

MarquessV
Copy link
Contributor

@MarquessV MarquessV commented Aug 21, 2024

closes #385

TODO

  • Apply changes based on this discussion. Until then, CI is not expected to pass.

Copy link

github-actions bot commented Aug 21, 2024

PR Preview Action v1.4.7
🚀 Deployed preview to https://rigetti.github.io/quil-rs/pr-preview/pr-390/
on branch quil-py-docs at 2024-08-22 18:45 UTC

quil-rs/src/parser/lexer/mod.rs Outdated Show resolved Hide resolved
quil-rs/src/program/mod.rs Outdated Show resolved Hide resolved
token_with_location(value(Token::Indentation, tag(" "))),
preceded(many0(tag(" ")), lex_token),
),
(lex_indent, preceded(many0(tag(" ")), lex_token)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking we should remove the preceded(many0(tag(" ")) part, since we're being more strict about indentation being four spaces.

Suggested change
(lex_indent, preceded(many0(tag(" ")), lex_token)),
(lex_indent, lex_token),

Copy link
Contributor Author

@MarquessV MarquessV Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was expecting to remove that too, but it becomes clear pretty quickly that the preceded parser in question is actually necessary to catch spaces between instructions tokens.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a concrete example of this? Instructions should not be separated by spaces afaik

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, instructions was the wrong word there, I meant tokens. Having an issue with my environment, but I'll share an example of how removing the preceded parser causes an error.

Copy link
Contributor

@Shadow53 Shadow53 Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that makes more sense. In that case, it seems like separated_list0 or separated_list1 seems more correct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I could be missing my shot in nom golf, but I can't figure out how to make that combinator work for this case. It doesn't satisfy the trait bounds of the other wrapping combinators.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe my intent was something like this, which does change the signature of the right-hand parser from TokenWithLocation to Vec<TokenWithLocation>, but also seems more correct?

Suggested change
(lex_indent, preceded(many0(tag(" ")), lex_token)),
(lex_indent, separated_list1(space1, lex_token)),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose, in order to make this "more correct", newline parsing would have to be handled by lex_indent (since an indent is specifically a newline followed by four spaces). That seems like it would cause more code changes, since it seems like newlines are currently handled by lex_token?

quil-rs/src/parser/lexer/mod.rs Show resolved Hide resolved
@MarquessV MarquessV requested a review from Shadow53 August 30, 2024 18:51
@MarquessV MarquessV merged commit f8cc41e into main Sep 11, 2024
14 checks passed
@MarquessV MarquessV deleted the indentation-parsing branch September 11, 2024 18:18
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.

The parser is not robust to whitespace
2 participants