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

allow whitespace before closing > #2021

Merged
merged 2 commits into from
Jun 22, 2018
Merged

Conversation

anmonteiro
Copy link
Member

fixes #2020

@@ -538,9 +538,9 @@ rule token = parse
set_lexeme_length lexbuf 1;
LBRACE
}
| "</" uppercase_or_lowercase (identchar | '.') * ">" {
| "</" uppercase_or_lowercase (identchar | '.') * blank* ">" {
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're add it, should we accept </ *space* div > too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point. Adding now

Copy link
Member Author

@anmonteiro anmonteiro Jun 22, 2018

Choose a reason for hiding this comment

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

@IwanKaramazow I'm actually not sure if that's possible given ></ is a valid infix operator.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's trivial to allow <div> </ div > (note the space between > and </, but <div></ div> would be really tricky, I think.

Copy link
Contributor

@IwanKaramazow IwanKaramazow Jun 22, 2018

Choose a reason for hiding this comment

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

Right, I see the problem, nevermind, this wouldn't benefit the quality of the code much.

Copy link
Member Author

@anmonteiro anmonteiro Jun 22, 2018

Choose a reason for hiding this comment

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

Alright, pushing the trivial case though because that one is easy.

let buf = Lexing.lexeme lexbuf in
LESSSLASHIDENTGREATER (String.sub buf 2 (String.length buf - 2 - 1))
LESSSLASHIDENTGREATER (String.trim (String.sub buf 2 (String.length buf - 2 - 1)))
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated question: how many tries did it take you to get this right? This string api is the worst

Copy link
Member Author

Choose a reason for hiding this comment

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

hahaha. I typed String. and searched for trim. Merlin helped me there, so 1 try!

Copy link
Member

Choose a reason for hiding this comment

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

But the calculation, etc? String.sub throws so I'm always uneasy with off-by-ones

Copy link
Member Author

Choose a reason for hiding this comment

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

The calculation was there already. I'm just trimming the result. What the calculation does is remove "</" and ">" from what we lexed. I'm trimming that result.

Copy link
Member

Choose a reason for hiding this comment

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

Ah sry misread, thanks.

@chenglou
Copy link
Member

2fast4me

@chenglou chenglou merged commit bc2240b into reasonml:master Jun 22, 2018
@anmonteiro anmonteiro deleted the gh-2020 branch June 22, 2018 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<div></div > fails
4 participants