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

Derive Debug for FontGroup and its components #8908

Closed
wants to merge 2 commits into from
Closed

Derive Debug for FontGroup and its components #8908

wants to merge 2 commits into from
+20 −9

Conversation

@antrik
Copy link
Contributor

antrik commented Dec 9, 2015

In order to enable #[derive(Debug)] on HashMap (used by Font), this also requires applying trait bounds to HashMap itself (rather than just the impl) -- which is probably better anyway.

Review on Reviewable
@highfive
Copy link

highfive commented Dec 9, 2015

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

Ms2ger commented Dec 9, 2015

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Dec 9, 2015

📌 Commit 31858db has been approved by Ms2ger
@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 9, 2015

Thank you for your PR!
@bors-servo
Copy link
Contributor

bors-servo commented Dec 9, 2015

⌛ Testing commit 31858db with merge c5f9d78...
bors-servo added a commit that referenced this pull request Dec 9, 2015
@bors-servo
Auto merge of #8908 - antrik:debug-fontgroup, r=Ms2ger
c5f9d78 
Derive Debug for FontGroup and its components

In order to enable `#[derive(Debug)]` on HashMap (used by Font), this also requires applying trait bounds to HashMap itself (rather than just the impl) -- which is probably better anyway.

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

bors-servo commented Dec 9, 2015

💔 Test failed - mac-dev-ref-unit
@antrik
Copy link
Contributor Author

antrik commented Dec 9, 2015

This is bad: The macos platform implementation ultimately goes back to core_font::CTFont, which doesn't implement Debug... Not sure how to deal with this. Implement it in core_font, and depend on the updated version? (This is kinda getting out of hand, for a simple debug convenience feature...) Or maybe just add a dummy impl Debug for CTFont for now in the files using it?...
@antrik
Copy link
Contributor Author

antrik commented Dec 9, 2015

Also, don't know how to test the macos build without running that system myself?...
@jdm
Copy link
Member

jdm commented Dec 9, 2015

We can run @bors-servo: try to run a sample compile against linux, OS X, and android, if necessary.
@antrik antrik mentioned this pull request Dec 10, 2015
Merged
bors-servo added a commit to servo/core-text-rs that referenced this pull request Dec 10, 2015
@bors-servo
Auto merge of #45 - antrik:debug, r=<try>
Loading status checks…
51183a7 
Derive Debug for CTFont, CTFontCollection, CTFontDescriptor

These are all the structs as far as I can see.

Note that this is entirely untested... Let's see whether I got it right :-)

(Needed for servo/servo#8908 )

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/core-text-rs/45)
<!-- Reviewable:end -->
bors-servo added a commit to servo/core-text-rs that referenced this pull request Dec 11, 2015
@bors-servo
Auto merge of #45 - antrik:debug, r=metajack
Loading status checks…
a9361cb 
Derive Debug for CTFont, CTFontCollection, CTFontDescriptor

These are all the structs as far as I can see.

Note that this is entirely untested... Let's see whether I got it right :-)

(Needed for servo/servo#8908 )

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/core-text-rs/45)
<!-- Reviewable:end -->
@antrik antrik force-pushed the antrik:debug-fontgroup branch from 31858db to a15ed61 Dec 11, 2015
@antrik
Copy link
Contributor Author

antrik commented Dec 11, 2015

I added the platform/macos bits. It's entirely untested, though -- let's see what the build bot makes of it...
@antrik antrik force-pushed the antrik:debug-fontgroup branch from a15ed61 to e793801 Dec 11, 2015
@antrik
Copy link
Contributor Author

antrik commented Dec 12, 2015

Looks good so far... Will it actually build on MacOS? :-)
@KiChjang
Copy link
Member

KiChjang commented Dec 12, 2015

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Dec 12, 2015

⌛ Trying commit e793801 with merge d58ee70...
bors-servo added a commit that referenced this pull request Dec 12, 2015
@bors-servo
Auto merge of #8908 - antrik:debug-fontgroup, r=<try>
d58ee70 
Derive Debug for FontGroup and its components

In order to enable `#[derive(Debug)]` on HashMap (used by Font), this also requires applying trait bounds to HashMap itself (rather than just the impl) -- which is probably better anyway.

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

bors-servo commented Dec 12, 2015

💔 Test failed - mac-rel-wpt
@KiChjang
Copy link
Member

KiChjang commented Dec 12, 2015

@antrik your PR compiles fine right now, it's just the intermittents that's causing an issue.
@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 17, 2015

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Dec 17, 2015

⌛ Trying commit c5ede58 with merge 4327fa9...
bors-servo added a commit that referenced this pull request Dec 17, 2015
@bors-servo
Auto merge of #8908 - antrik:debug-fontgroup, r=<try>
4327fa9 
Derive Debug for FontGroup and its components

In order to enable `#[derive(Debug)]` on HashMap (used by Font), this also requires applying trait bounds to HashMap itself (rather than just the impl) -- which is probably better anyway.

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

Ms2ger commented Dec 17, 2015

Doh.

@bors-servo try- force r+
@bors-servo
Copy link
Contributor

bors-servo commented Dec 17, 2015

📌 Commit c5ede58 has been approved by Ms2ger
@frewsxcv frewsxcv closed this Dec 17, 2015
@frewsxcv frewsxcv reopened this Dec 17, 2015
@nox
Copy link
Member

nox commented Dec 17, 2015

@bors-servo r- force
@nox
Copy link
Member

nox commented Dec 17, 2015

@bors-servo r=Ms2ger
@bors-servo
Copy link
Contributor

bors-servo commented Dec 17, 2015

📌 Commit c5ede58 has been approved by Ms2ger
@frewsxcv frewsxcv closed this Dec 17, 2015
@frewsxcv frewsxcv reopened this Dec 17, 2015
@frewsxcv frewsxcv closed this Dec 17, 2015
@frewsxcv frewsxcv reopened this Dec 17, 2015
@frewsxcv frewsxcv closed this Dec 17, 2015
@frewsxcv frewsxcv reopened this Dec 17, 2015
@KiChjang KiChjang closed this Dec 17, 2015
@KiChjang KiChjang reopened this Dec 17, 2015
@nox
Copy link
Member

nox commented Dec 17, 2015

@bors-servo r- force
1 similar comment
@nox
Copy link
Member

nox commented Dec 17, 2015

@bors-servo r- force
@KiChjang KiChjang closed this Dec 17, 2015
@KiChjang KiChjang reopened this Dec 17, 2015
@nox nox closed this Dec 17, 2015
@nox nox mentioned this pull request Dec 17, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Ms2ger Ms2ger

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@antrik @highfive @Ms2ger @bors-servo @jdm @KiChjang @nox @frewsxcv
You can’t perform that action at this time.