Add Team Drive model, basic fetch (no File search query string) and fetch test.
Add Team Drive usage example that allows listing Team Drives and their contents (very crude). Uses teamdrivefetchjob and filefetchjob.
Details
- Reviewers
dvratil - Maniphest Tasks
- T10521: [Drive] Implement Team Drives portion of the API
- Commits
- R477:0f3edc914b45: Add Team Drive model and basic fetch.
See provided example in examples/teamdrive.
Diff Detail
- Repository
- R477 KGAPI Library
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Nice, thanks for the patch!
I haven't checked the code in-depth (I don't know if I'll get to it before the weekend, sorry), but one thing that I noticed: please update year and your name in the copyright headers :)
Second thing: I realized when I was fixing a bug in kgapi recently that having all the properties as strings everywhere (e.g. QStringLiteral("fooPropertyName") is annoying) - it's hard to read and easy to make a typo in. Could you please define all the strings that are used as JSON property names and URL queries as named constants in an anonymous namespace at the top of the file, e.g. for TeamDrive:
namespace { static const QString tdId = QStringLiteral("id"); static const QString tdName = QStringLiteral("name"); ... }
Same for URL query parameters in jobs, e.g. static const QString queryMaxResults = QStringLiteral("maxResults");
I will eventually get around to change it everywhere to make the code safer. It will also reduce the size of the binaries since the strings won't be repeated twice there.
Thanks!
Nice job!
There's a bunch of places where you can simplify the code by using C++11 default initialization and defaulted constructors and destructors, as well as QScopedPointer to own the d-pointers everywhere. I realize we don't use them elsewhere, but this is probably the first big contribution since we switched to C++11, so let's set an example for any future contributions :) I'll eventually get around and clean up the rest of the code as well.
Once you fix those, it's good to go.
src/drive/teamdrive.cpp | ||
---|---|---|
79 | Since the ctor here does not do anything special, you can just use Private() = default; to let the compiler to generate default implementation of the constructor (and you don't have to define it yourself later on) | |
80 | Same for the copy ctor, you can just use Private(const Private &other) = default and the compiler will generate the copy ctor for you, so you can remove the custom implementation. | |
82 | Initialize all the members to false, please. | |
110 | While we don't need to delete the d pointer ourselves when using QScopedPointer, the dtor still has to be declared here, but you can use Teamdrive::Restrictions::~Restrictions() = default; to let the compiler generate a default dtor. | |
149 | Private() = default; | |
150 | Private(const Private &) = default; | |
152 | Default-initialize all the members to false. | |
208 | Teamdrive::Capabilities::~Capabilities() = default; | |
331 | = default | |
332 | = default | |
335 | qreal is defined as a double by default and according to docs, those a float numbers, so let's just make those floats, and default-initialize them to 0.0f | |
362 | = default | |
401 | = default | |
402 | = default | |
442 | auto teamdrive = TeamdrivePtr::create() The idea when using smart pointers is to not have any new in your code :) | |
451 | BackgroundImageFilePtr::create() instead of new | |
459 | CapabilitiesPtr::create() instead of new | |
481 | RestrictionsPtr::create() instead of new | |
503 | = default | |
src/drive/teamdrive.h | ||
60 | Copy constructors are usually not explicit. | |
61 | Doesn't need to be virtual, we have no virtual methods and this is likely a final class. | |
97 | use QScopedPointer<Private> const d to have the d-pointer deleted automatically when the parent object is destroyed. Note that because Private is only forward-declared here, you still need to have the destructor for this class defined in the .cpp file, because that's where the compiler will generate destructor code for the QScopedPointer as well (and where Private is already defined). | |
111 | Remove explicit | |
112 | Remove virtual | |
228 | QScopedPointer<Private> | |
243 | Remove explicit | |
244 | Remove virtual | |
272 | QScopedPointer<Private> | |
280 | Remove explicit. | |
337 | QScopedPointer<Private> | |
src/drive/teamdrivefetchjob.cpp | ||
51 | Default-Initialize to 0 here instead of the ctor please. | |
52 | Default-initialize to false here instead of the ctor please. | |
55 | TeamdriveFetchJob * const q | |
90 | = default | |
src/drive/teamdrivefetchjob.h | ||
84 | QScopedPointer |
Use C++11 default constructors and QScopedPointer. I should have fixed all the points, let me know if I missed something.