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

Make `Tokenizer` public #74

Closed
wants to merge 1 commit into from
Closed

Make `Tokenizer` public #74

wants to merge 1 commit into from

Conversation

@xojoc
Copy link

xojoc commented May 24, 2015

Review on Reviewable

xojoc
@SimonSapin
Copy link
Member

SimonSapin commented May 24, 2015

Why? What's missing in Parser?

(In general, when requesting a change, it's helpful to explain the motivation in the request.)

@xojoc
Copy link
Author

xojoc commented May 24, 2015

Parser.next skips block tokens and I couln't find a way to parse them. I tried to use parse_nested_block but I just get a lot of lifetime errors.
If this change is inaceptable, could you please give me an example of how to parse blocks (and yes I've already read the Servo code.)

@SimonSapin
Copy link
Member

SimonSapin commented May 25, 2015

It’s not necessarily unacceptable but it should be justified and this doesn’t seem like a strong enough justification.

The intended usage is that when Parser::next returns a block or function, you can parse its content with Parser::parse_nested_block and a closure that takes a new Parser object as a parameter. The closure should use that object, not the one parse_nested_block was called on. The inner parser will "end" (next will return Err(())) at the end of the block or function. Examples:

https://github.com/servo/rust-cssparser/blob/72cf5e4ea8/src/rules_and_declarations.rs#L389
https://github.com/servo/rust-cssparser/blob/72cf5e4ea8/src/color.rs#L68-L70

This takes care of block nesting (and spec-compliant error recovery) for you. When using the tokenizer directly, you’d have to keep track yourself of a stack of opened blocks to find the matching closing token.

I’d consider making the tokenizer public if there is a use case for opting out completely of this block nesting handling.

@SimonSapin
Copy link
Member

SimonSapin commented Sep 2, 2015

@xojoc, does my comment above help?

@xojoc
Copy link
Author

xojoc commented Sep 2, 2015

Yes, thank you :) I just needed to get more familiar with Rust.

@xojoc xojoc closed this Sep 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.