Skip to content

/ servo

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

Added check for the existence of for and element attribute when preparing a script #5274

Closed
wants to merge 3 commits into from
Closed

Added check for the existence of for and element attribute when preparing a script #5274

wants to merge 3 commits into from

Conversation

@KiChjang
Copy link
Member

KiChjang commented Mar 19, 2015

Fixes #5258
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Mar 19, 2015

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

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.
@KiChjang KiChjang force-pushed the KiChjang:script-for-event-attribute branch from 25028ec to 2d27338 Mar 27, 2015
@KiChjang
Added check for the existence of for and element attribute when prepa…
Loading status checks…
2d27338 
…ring a script
@KiChjang
Copy link
Member Author

KiChjang commented Mar 27, 2015

Squashed, r? @Ms2ger
@jdm

This comment has been minimized.

Sign in to view
Copy link

jdm commented on 2d27338 Mar 27, 2015

r=Ms2ger
@jdm jdm added S-awaiting-merge and removed S-needs-squash labels Mar 27, 2015
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented on 2d27338 Mar 27, 2015

saw approval from Ms2ger
at KiChjang@2d27338

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Mar 27, 2015

merging KiChjang/servo/script-for-event-attribute = 2d27338 into auto

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Mar 27, 2015

KiChjang/servo/script-for-event-attribute = 2d27338 merged ok, testing candidate = dfecc15

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo replied Mar 27, 2015

some tests failed:
failure: http://build.servo.org/builders/linux2/builds/1177
bors-servo pushed a commit that referenced this pull request Mar 27, 2015
bors-servo
auto merge of #5274 : KiChjang/servo/script-for-event-attribute, r=Ms…
Loading status checks…
dfecc15 
…2ger

Fixes #5258
@Ms2ger
Copy link
Contributor

Ms2ger commented Mar 28, 2015

Looks like there are issues with the rooting. Please try putting the roots in local bindings and matching on (for.r(), event.r()).
@KiChjang
Added r() to properly root the for/event attributes
Loading status checks…
61621ae
@Ms2ger
Copy link
Contributor

Ms2ger commented Mar 29, 2015

I'm going to have to ask you to build before pushing.
@KiChjang
Fixed the usage of r() within the code block
Loading status checks…
99d8782
@metajack
Copy link
Contributor

metajack commented Mar 31, 2015

Assigned to @Ms2ger since he was already reviewing this.
@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 1, 2015

Rebased as #5474
@Ms2ger Ms2ger closed this Apr 1, 2015
@Ms2ger Ms2ger removed their assignment Apr 1, 2015
@KiChjang KiChjang deleted the KiChjang:script-for-event-attribute branch Apr 1, 2015
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-review
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Script for/event attributes should only be considered if they're both present.
6 participants
@KiChjang @hoppipolla-critic-bot @Ms2ger @metajack @bors-servo @jdm
You can’t perform that action at this time.