Summary: | [GTK] Print API missing documentation when generating gtkdoc | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||
Component: | WebKitGTK | Assignee: | Martin Robinson <mrobinson> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cgarcia, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 523.x (Safari 3) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Martin Robinson
2012-09-03 17:18:24 PDT
Created attachment 161953 [details]
Patch
Comment on attachment 161953 [details]
Patch
Patch looks good to me, but it seems there are two independent changes, one is raising an exception when rebase is called before doc is generated, instead of just showing a message, I guess to make sure the script fails in such case, because the exception is not handled. And the other change is showing missing api when errors are reported. You could you split the patch before landing, please?
(In reply to comment #2) > (From update of attachment 161953 [details]) > Patch looks good to me, but it seems there are two independent changes, one is raising an exception when rebase is called before doc is generated, instead of just showing a message, I guess to make sure the script fails in such case, because the exception is not handled. And the other change is showing missing api when errors are reported. You could you split the patch before landing, please? In this case, it was a small cleanup that I did while fixing the issue. If you feel it's serious enough that it needs to be in another patch, I'm happy to do that. Can I have a review in advance though? (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 161953 [details] [details]) > > Patch looks good to me, but it seems there are two independent changes, one is raising an exception when rebase is called before doc is generated, instead of just showing a message, I guess to make sure the script fails in such case, because the exception is not handled. And the other change is showing missing api when errors are reported. You could you split the patch before landing, please? > > In this case, it was a small cleanup that I did while fixing the issue. If you feel it's serious enough that it needs to be in another patch, I'm happy to do that. Can I have a review in advance though? I already set r+ (In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 161953 [details] [details] [details]) > > > Patch looks good to me, but it seems there are two independent changes, one is raising an exception when rebase is called before doc is generated, instead of just showing a message, I guess to make sure the script fails in such case, because the exception is not handled. And the other change is showing missing api when errors are reported. You could you split the patch before landing, please? > > > > In this case, it was a small cleanup that I did while fixing the issue. If you feel it's serious enough that it needs to be in another patch, I'm happy to do that. Can I have a review in advance though? > > I already set r+ I'm still not sure if you consider the cleanup serious enough to split after my comment. Comment on attachment 161953 [details]
Patch
Oops. Guess I'll wait on this one. :)
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > (From update of attachment 161953 [details] [details] [details] [details]) > > > > Patch looks good to me, but it seems there are two independent changes, one is raising an exception when rebase is called before doc is generated, instead of just showing a message, I guess to make sure the script fails in such case, because the exception is not handled. And the other change is showing missing api when errors are reported. You could you split the patch before landing, please? > > > > > > In this case, it was a small cleanup that I did while fixing the issue. If you feel it's serious enough that it needs to be in another patch, I'm happy to do that. Can I have a review in advance though? > > > > I already set r+ > > I'm still not sure if you consider the cleanup serious enough to split after my comment. Not because it's serious, but because it's unrelated change, but I don't mind if you commit both in a single commit, I was just suggesting to split it before landing, no need to file new bug reports and patches. But still, feel free to cq+, I don't want to give you more work :-P Comment on attachment 161953 [details]
Patch
Okay. Thanks. I'll try to be more careful in the future.
Comment on attachment 161953 [details] Patch Clearing flags on attachment: 161953 Committed r127475: <http://trac.webkit.org/changeset/127475> All reviewed patches have been landed. Closing bug. |