D19948: DRAFT Add Standard Query Parameters support

[Daniel Vrátil]

dvratil requested changes to this revision.
dvratil added a comment.
This revision now requires changes to proceed.


  Nice! I think this is a great thing to have in libkgapi, thanks for looking into this.
  
  Some API design related comments below, but the approach is generally OK.

INLINE COMMENTS

> aboutfetchjobtest.cpp:84
> +        if (strcmp(LimitedFieldTag, QTest::currentDataTag()) == 0) {
> +            job->setFields(PermissionIdField);
> +        }

Should we maybe provide the fields as string constants in the relevant job APIs (or elsewhere)?

> job.cpp:433
> +
> +void Job::setFields(const QString& fields)
> +{

Coding style: `Type &var` instead of `Type& var`

> job.h:188
> +     */
> +    void setFields(const QString &fields);
> +

IMO should be a `QStringList` to allow usage like

  job->setFields({Field1, Field2, Field3});

Same for the getter. We can concatenate the fields just before serializing them into the query ourselves, but the API will be nicer for users.

> job.h:193
> +     *
> +     * @return Am Comma separated list of fields
> +     */

Extra `Am` ?

> job.h:202
> +     */
> +    QList<QPair<QString, QString> > getStandardQueryParams() const;
> +

Use `QVector` instead of a `QList`, please. You can drop the space between `> >`, too. C++11 fixed the grammar, so it's not misparsed as a streaming operator anymore.

Also, create a custom `struct` with properly named members (`query`, `value`, maybe?) instead of using a `QPair`. I know the `QUrlQuery` API uses that, but let's not be lazy just because Qt is :-)

> driveservice.cpp:43
>  
> -QUrl fetchAboutUrl(bool includeSubscribed, qlonglong maxChangeIdCount, qlonglong startChangeId)
> +QUrl fetchAboutUrl(bool includeSubscribed, qlonglong maxChangeIdCount, qlonglong startChangeId, QList<QPair<QString, QString> > standardQueryParams)
>  {

Pass complex types as `const &`. It doesn't make much difference since Qt containers are implicitly shared, but it's a convention in Qt-based code.

REPOSITORY
  R477 KGAPI Library

REVISION DETAIL
  https://phabricator.kde.org/D19948

To: barchiesi, dvratil
Cc: kde-pim, #libkgapi, barchiesi, gennad, dvasin, rodsevich, winterz, vkrause, mlaurent, knauss, dvratil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20190322/d1e9f8b3/attachment.html>