-
Notifications
You must be signed in to change notification settings - Fork 443
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
Add cmake improvements blogpost #121
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple minor tweaks
Addressed comments, unless there is more concerns, this is good for final review and I'll merge after that. The preview link here: |
@jtattermusch Could you rebase against |
Co-Authored-By: Zack Galbreath <zack.galbreath@kitware.com>
6bf620b
to
71a80c8
Compare
Rebased. |
It this good to merge now? |
The "View File" preview is still showing
If that's intended, or will otherwise be replaced, then ignore this commit. Otherwise everything LGTM! |
In this preview link you can see that it's rendered correctly: |
@lucperkins @thisisnotapril can you approve so I can merge? |
A quick copyediting pass would be needed, but that can be done after this is merged. I'll let Luc give the final thumbs up. |
author: Zack Galbreath | ||
author-link: https://github.com/zackgalbreath | ||
date: "2020-03-06T00:00:00Z" | ||
published: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think that we need a this published: true
entry.
I'm going to make some of the requested non-content changes myself (filename, params, etc.) |
A have a few more TODOs in the text, but once I resolve them, this can be published.