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

script::dom::servoparser::async_html::Tokenizer uses Err for a non-errror condition #25516

Open
pshaughn opened this issue Jan 13, 2020 · 4 comments · May be fixed by #25623
Open

script::dom::servoparser::async_html::Tokenizer uses Err for a non-errror condition #25516

pshaughn opened this issue Jan 13, 2020 · 4 comments · May be fixed by #25623

Comments

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Jan 13, 2020

This functionally works, but it's misleading:

pub fn feed(&mut self, input: &mut BufferQueue) -> Result<(), DomRoot<HTMLScriptElement>> {

Returning an HTMLScriptElement is just a normal case here, not an error condition. If Option<DomRoot<HTMLScriptElement>> is undesirable, an enum would be more appropriate than using Result in this way.

@pshaughn pshaughn changed the title script::dom::servoparser::async::html::Tokenizer uses Err for a non-errror condition script::dom::servoparser::async_html::Tokenizer uses Err for a non-errror condition Jan 13, 2020
@nipunG314
Copy link

@nipunG314 nipunG314 commented Jan 27, 2020

I would like to refactor this.

Is there any particular reason why an Option here would be undesirable? Also, the feed function in html.rs has the same return type. Should I refactor that as well?

@jdm
Copy link
Member

@jdm jdm commented Jan 27, 2020

I suspect that Result was used for the benefit of the #[must_use] attribute. It's not legal to use #[must_use] on a function, not just a type, so we can do that instead (https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=5cdb5158697dca6a565185a520b1d499), and use an Option type. And yes, please change the feed function in html.rs.

@nipunG314
Copy link

@nipunG314 nipunG314 commented Jan 27, 2020

Got it. And are there any relevant tests for the servoparser? Or would a successful build be sufficient?

@jdm
Copy link
Member

@jdm jdm commented Jan 27, 2020

A successful build will be sufficient.

@nipunG314 nipunG314 linked a pull request that will close this issue Jan 27, 2020
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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