Code review comment for lp://staging/~michihenning/unity-scopes-api/qt-coverage

Revision history for this message
Michi Henning (michihenning) wrote :

> 1235 +// TODO: These are in the public API, so they need documentation.
>
> Good point; should they actually be part of the public API? I think we're
> hiding scopes::Variant everywhere and only expose QVariant and do the
> conversions internally. Would it make sense to hide them instead?
>
> 766 + * Copyright (C) 2013 Canonical Ltd
> 2353 + * Copyright (C) 2014 Canonical Ltd
>
> Bump the year please (there are more of them, just pasting two examples) :)

I've gone through and checked them all. I found one that had 2013 instead of 2014 (HttpAsyncReader.h). The others all have 2014. I think that's right, seeing that Xavi was working on this last year.

> 35 +unity-scopes-api (0.6.15+15.04.20150213-0ubuntu2) UNRELEASED;
> urgency=medium
>
> The beloved changelog! Should say 0.6.15-0ubuntu1 ;)

Ah, my good old friend, the changelog... Neither "dch" nor "dch -i" does the right thing. Is there a magic incantation that does?

> > I think it would probably make sense to put _ENABLE_QT_EXPERIMENTAL_ in a
> separate include file, and just #include that everywhere, but feel free to
> leave it as is; since it's a temporary measure there is no need to put more
> effort into changing all that again. On second thought, you probably had a
> good reason to copy-paste it everywhere (better error message from the
> compiler about offending include file?) ;)

Yes, that's one reason. Also, I don't like "artificial" public headers. I've removed the EXPERIMENTAL thingy from all the internal header files though. It's not needed there; we only need it in the public headers.

> I'm a bit baffled by the amount of changes to symbols file; is it just about
> re-sorting them?

Yes. It turns out that the qt lib wasn't processed by the symbol_diff script, so we didn't get automatic diffs if bzr bd complained. I added processing for the qt lib to the script, so that's fixed now. The script sorts the symbol file and puts it through uniq (it has to because, otherwise, we end up with more and more duplicate lines in the file), which is why there is that large diff.

I've moved the QUtils functions into the internal namespace. (That also caused a change in the symbols file.)

« Back to merge proposal