Add support for parsing WOFF2 fonts. #14
Conversation
|
I've added a few comments via redpen. I have to admit, I don't find that the easiest, because it makes things like searching hard, and I'm used to reviewing APIs in code. But I've given it a go :) General comment, is this seems separate from ConradIrwin/font in a few places, doesn't depend on same types, such as Tag. This is ok but just wanted to explicitly call that out. |
@bramp, thank you for the review and for playing along with my experiment! It has been very insightful and helpful to me. In hindsight, I can see that showing you only the godoc view to review is suboptimal in many ways. Not being able to search because it was an image (redpen is meant for design reviews; I was kinda abusing it) is an important one, thanks for pointing it out. But more than that, I've realized that there are many more relevant details in code that don't make it into the
So now I think that showing you the I've replied to your existing comments on redpen, but in the meantime, could you please take another look at the actual code change (with commit message and TODO comments visible): https://dmitri.shuralyov.com/font/woff2/...$changes/1 (Please be aware that my code review tool is still a work in progress and the commenting functionality is not finished. You can only view the change—but can't leave review comments on it yet. If you don't mind, just leave comments here instead. I realize the line numbers aren't being shown yet, so it might be hard... Thanks and sorry for the unusual setup; this is an active project of mine.) 😄 |
|
Friendly ping. Let me know if there's anything I can do to help make progress on this. If there are any concerns about the approach, just let me know. |
|
@shurcooL Are you happy with the woff2 library yet? I'm happy for this to depend on that if you're not going to change the API anymore :) |
|
@ConradIrwin It is ready from my side. I've responded to all review comments so far, and I was just waiting in case there would be any further review comments. I'll go ahead and merge that change now. Then, I'll resolve the conflicts in this PR (it had some conflicts with #15) so it can be merged.
I'm not going to change the API in the short term, but it's best to think of it as v0 API. I do want to evolve it as I make further progress on the encoder and discover opportunities for API improvement. If there are any changes to its APIs, I will be sending PRs here to update the code and keep |
|
Sounds good
I like the SLA idea :D I should figure out what I can commit to, but it might be embarrassingly low...
Sent via Superhuman ( https://sprh.mn/[email protected] )
…On Tue, Feb 20, 2018 at 12:10 PM, Dmitri Shuralyov < ***@***.*** > wrote:
@ ConradIrwin ( https://github.com/conradirwin ) It is ready from my side.
I've responded to all review comments so far, and I was just waiting in
case there would be any further review comments.
I'll go ahead and merge that change now. Then, I'll resolve the conflicts
in this PR (it had some conflicts with #15 (
#15 ) ) so it can be merged.
>
>
> if you're not going to change the API anymore
>
>
I'm not going to change the API in the short term, but it's best to think
of it as v0 API. I do want to evolve it as I make further progress on the
encoder and discover opportunities for API improvement. If there are any
changes to its APIs, I will be sending PRs here to update the code and
keep ConradIrwin/font building. See https:/ / github. com/ shurcooL/ SLA#breaking-api-changes
( https://github.com/shurcooL/SLA#breaking-api-changes ) for more
information about my approach.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub (
#14 (comment) ) , or mute
the thread (
https://github.com/notifications/unsubscribe-auth/AAFwQF6DLFSwNXV_YhR9aaWpqFzx1Oyvks5tWybBgaJpZM4R9Rgc
).
|
|
I've merged https://dmitri.shuralyov.com/font/woff2/...$changes/1. Going to resolve conflicts here now. |
This change uses the font/woff2 package for parsing WOFF2 font files. Update all parts of the documentation, usage, etc., to say that WOFF2 fonts are now supported. Add test .woff2 file to testdata and run smoke test on it. Add benchmarks for parsing WOFF2 font files. Fix minor Go style and documentation issues. E.g., "Woff" should be "WOFF" because of https://golang.org/s/style#initialisms. Resolves #1.
|
I've rebased and resolved conflicts. PTAL. |
|
looks good, thanks! |
DO NOT MERGE. This PR depends on font/woff2#1 being merged first.This PR is open for review and discussion.This PR implements support for WOFF2 files and resolves #1 by using a dedicated
woff2parser package.Thewoff2package isn't live yet, it still needs review. First, I suggest reviewing its public API. Once that looks good, we can move on to reviewing its internal code. You can see and review its API on redpen:https://redpen.io/rk9a75c358f45654a8(If you don’t have have a redpen account and would rather not make one for posting review comments, feel free to post them here instead.)Edit: See my comment below for an update.
This PR is meant to continue the conversation from #1. /cc @ConradIrwin @bramp