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.

Sign up for GitHub

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

Jump to bottom

Issue4484 to js val convertible for str #4496

Merged
merged 2 commits into from
Jan 1, 2015
Merged

Issue4484 to js val convertible for str #4496

merged 2 commits into from
Jan 1, 2015

Conversation

yodalee
Copy link
Contributor

@yodalee yodalee commented Dec 28, 2014

#4484

Add ToJSValConvertible trait to str type.

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/3604

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@highfive
Copy link

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@jdm jdm added the S-awaiting-review There is new code that needs to be reviewed. label Dec 28, 2014
@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 29, 2014

@yodalee: thanks! I reviewed on Critic; just one nit.

@Ms2ger Ms2ger added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 29, 2014
@yodalee yodalee force-pushed the issue4484-ToJSValConvertible-for-str branch from 4d82582 to 3647ed3 Compare December 29, 2014 12:36
@yodalee
Copy link
Contributor Author

yodalee commented Dec 29, 2014

Fix, I amend the last commit.

@yodalee yodalee force-pushed the issue4484-ToJSValConvertible-for-str branch from 3647ed3 to 0ba9936 Compare December 29, 2014 22:24
@yodalee
Copy link
Contributor Author

yodalee commented Dec 31, 2014

CI said it has merge conflicts, should I rebase this branch to newest master?

@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 31, 2014

Please do, thanks.

@yodalee yodalee force-pushed the issue4484-ToJSValConvertible-for-str branch from 0ba9936 to 2509da0 Compare December 31, 2014 12:28
@yodalee
Copy link
Contributor Author

yodalee commented Dec 31, 2014

rebase done

@jdm jdm added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Dec 31, 2014
@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 31, 2014

@yodalee: looks good, except that you changed the 4-space indentation back to 2-space indentation (as it was when you first created the PR) in CodegenRust.py. Could you make that 4 spaces once more? Thanks!

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 31, 2014
@yodalee
use str to_jsval() now
bb087c2 
xmlhttprequest.rs, CodegenRust.py
replace into_string().to_jsval() to to_jsval()

conversions.rs
DOMString to_jsval() use as_slice().tojsval() now
@yodalee yodalee force-pushed the issue4484-ToJSValConvertible-for-str branch from 2509da0 to bb087c2 Compare December 31, 2014 18:18
@yodalee
Copy link
Contributor Author

yodalee commented Dec 31, 2014

My fault, fix now.

@jdm jdm added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Dec 31, 2014
@jdm
Copy link
Member

jdm commented Dec 31, 2014

Thanks!

bors-servo pushed a commit that referenced this pull request Dec 31, 2014
auto merge of #4496 : yodalee/servo/issue4484-ToJSValConvertible-for-…
aaf099c 
…str, r=jdm

#4484 
Add ToJSValConvertible trait to str type.
@yodalee
Copy link
Contributor Author

yodalee commented Jan 1, 2015

Rust blow up O口O

bors-servo pushed a commit that referenced this pull request Jan 1, 2015
auto merge of #4496 : yodalee/servo/issue4484-ToJSValConvertible-for-…
fda38cf 
…str, r=jdm

#4484 
Add ToJSValConvertible trait to str type.
@bors-servo bors-servo closed this Jan 1, 2015
@bors-servo bors-servo merged commit bb087c2 into servo:master Jan 1, 2015
@yodalee yodalee deleted the issue4484-ToJSValConvertible-for-str branch January 5, 2015 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@yodalee @hoppipolla-critic-bot @highfive @Ms2ger @jdm @Manishearth @bors-servo