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

Fix #9508: Beautify our union enums constructors #9543

Merged

Conversation

@alopatindev
Copy link
Contributor

alopatindev commented Feb 4, 2016

Solves #9508
@jdm Please review. Thanks!

Review on Reviewable

@highfive
Copy link

highfive commented Feb 4, 2016

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

@highfive
Copy link

highfive commented Feb 4, 2016

Heads up! This PR modifies the following files:

  • @ecoal95: components/script/dom/webglrenderingcontext.rs
@highfive
Copy link

highfive commented Feb 4, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm
Copy link
Member

jdm commented Feb 4, 2016

I'm actually going to hand this over to @nox, but thanks for doing this!
r? @nox

@alopatindev
Copy link
Contributor Author

alopatindev commented Feb 4, 2016

Ok, cool. Please note that I wasn't able to run tests neither on master nor on my branch (is that okay, actually?)

$ ./mach test
tidy reported no errors.
Error running mach:

    ['test']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

TypeError: test_ref() got an unexpected keyword argument 'kind'

  File "/home/sbar/git/servo/python/servo/testing_commands.py", line 152, in test
    Registrar.dispatch("test-%s" % suite, context=self.context, **kwargs)
  File "/home/sbar/git/servo/python/mach/mach/registrar.py", line 115, in dispatch
    return self._run_command_handler(handler, context=context, **kwargs)
  File "/home/sbar/git/servo/python/mach/mach/registrar.py", line 90, in _run_command_handler
    result = fn(**kwargs)

Thanks!


Review status: 0 of 13 files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Feb 4, 2016

I'll file an issue for that, but generally ./mach test will run far, far more tests than you ever want and will make your machine unusable for unrealistic periods of time. In this case, the fact that it compiles is enough to demonstrate validity of the change. For future changes, running particular test harnesses and subdirectories could make more sense (eg. ./mach test-wpt, ./mach test-css, ./mach test-unit).

@bors-servo
Copy link
Contributor

bors-servo commented Feb 5, 2016

The latest upstream changes (presumably #9521) made this pull request unmergeable. Please resolve the merge conflicts.

@alopatindev alopatindev force-pushed the alopatindev:enums_constructors_codingstyle_fix branch from 22fefd0 to 892ea30 Feb 5, 2016
@alopatindev
Copy link
Contributor Author

alopatindev commented Feb 5, 2016

That was my first rebase, actually. I fixed merge conflict and now I've got a single commit that should be mergeable with master. Hopefully I did it right :) Please check. Thanks


Review status: 0 of 112 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from the review on Reviewable.io

@KiChjang
Copy link
Member

KiChjang commented Feb 6, 2016

The rebase went horribly wrong. You currently have 14 unrelated commits in your PR. See https://github.com/servo/servo/wiki/Github-Workflow for a guide on how to rebase.

@alopatindev alopatindev force-pushed the alopatindev:enums_constructors_codingstyle_fix branch from 892ea30 to 2be4940 Feb 7, 2016
@alopatindev
Copy link
Contributor Author

alopatindev commented Feb 7, 2016

@KiChjang thanks for noticing that! I've removed unrelated commits using git rebase -i HEAD~15
Hopefully I didn't make it even worse :)
(But if I actually did—sorry; it's probably easier for me to recreate the PR then)


Review status: 0 of 13 files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@KiChjang KiChjang assigned KiChjang and unassigned nox Feb 7, 2016
@KiChjang
Copy link
Member

KiChjang commented Feb 7, 2016

Everything looks fine; thanks for addressing this issue!

@bors-servo r+

@KiChjang KiChjang closed this Feb 7, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2016

📌 Commit 2be4940 has been approved by KiChjang

@KiChjang KiChjang reopened this Feb 7, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2016

Testing commit 2be4940 with merge 28ecb0b...

bors-servo added a commit that referenced this pull request Feb 7, 2016
… r=KiChjang

Fix #9508: Beautify our union enums constructors

Solves #9508
@jdm Please review. Thanks!

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9543)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2016

💔 Test failed - mac-rel-css

@frewsxcv
Copy link
Member

frewsxcv commented Feb 7, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2016

Previous build results for android, gonk, linux-dev, mac-dev-unit, mac-rel-wpt are reusable. Rebuilding only linux-rel, mac-rel-css...

@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2016

💔 Test failed - linux-rel

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 7, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2016

Previous build results for android, gonk, linux-dev, mac-dev-unit, mac-rel-wpt are reusable. Rebuilding only linux-rel, mac-rel-css...

@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2016

@bors-servo bors-servo merged commit 2be4940 into servo:master Feb 7, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
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

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