The former fullColumnNames approach relied on the number and order of
columns returned by fullColumnNames to not change. Requesting the
required column explicitly makes this more robust.
Details
Details
- Reviewers
dvratil - Commits
- R165:5e8f6c1dd671: Add needed columns to QueryBuilder explicitly
Diff Detail
Diff Detail
- Repository
- R165 Akonadi
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Yay, I used arcanist to create a review request for the first time!
Please let me know
- if this should be pushed to Applications/17.08; it's obviously a bugfix, but I have no clue if it requires some push-to-stable preventing version bumps or something like that.
- if you spot some obvious mistakes I did while using arcanist.
Also, how do I specify PIM as reviewer using arcanist? Using "arc cover" only gave "Daniel Vrátil", which I couldn't use as reviewer as it differs from your user name. I correctly guessed that I have to specify "dvratil" instead, but I have no clue for "KDE PIM".
Comment Actions
TagTable has 4 columns, so qb.addColums() on l. 77 adds 4 columns (indices 0 to 3, but we skip 3 because it's tag type ID which we don't care about in the extraction), on line 81 we add the type name column, which is 5th column (index 4), and optionally we add 6th column (remoteId) with index 5, so the current code should be correct....
However it's very error-prone as if the columns in TagTable change, the code will indeed break, so please fix the way the columns are queries (see my inline comment). Then your change in the result extraction will be also correct, because we can skip querying the 4th columns (idx 3).
Regarding arcanist: I think it's even harder and more annoying to use than just uploading patches via web. I don't use it, so I can't give you any advice on how to trick it into actually being helpful, rather than just plain annoying :D
| src/server/handler/tagfetchhelper.cpp | ||
|---|---|---|
| 77 | Please replace this by qb.addColumn(Tag::idFullColumnName()); qb.addColumn(Tag::gidFullColumnName()); qb.addColumn(Tag::parentIdFullColumnName()); so that the indices don't get broken again if we add another column to the table. Then the rest of your patch will actually be correct :) | |
Comment Actions
I'll also update the commit message before I push this; my local changes somehow didn't make it through arcanist.
Last remaining question: which branch to commit against? master or Applications/17.08?
arcanist: I'll let you know when I got it figured out. I think you two had a bad start. To add PIM as reviewer using arcanist, you simply add <hashtag>kde_pim (putting an actual hashtag there in a comment automatically replaces it with the project; exactly what I was looking for ;-)