Merge lp:~kalikiana/u1db-qt/removeDoc into lp:u1db-qt

[Cris Dywan]

Implement Database.removeDoc method and use it in unit test

Functionally this is equivalent to replacing the doc with an empty one.

[Kevin Wright (kevin-wright-1)]

Isn't it that documents should always be null or empty and not removed?

If a document is null or empty it should be filtered out in Query or Index
should it not? Those are the mechanisms for filtering results, it seems
maybe the functionality should be there rather than Database, no?
On Nov 22, 2013 4:25 PM, "Christian Dywan" <email address hidden> wrote:

> Christian Dywan has proposed merging lp:~kalikiana/u1db-qt/removeDoc into
> lp:u1db-qt.
>
> Commit message:
> Implement Database.removeDoc method and use it in unit test
>
> Requested reviews:
> U1DB Qt developers (uonedb-qt)
> Related bugs:
> Bug #1243395 in U1DB Qt/ QML: "Need API to delete documents from QML"
> https://bugs.launchpad.net/u1db-qt/+bug/1243395
>
> For more details, see:
> https://code.launchpad.net/~kalikiana/u1db-qt/removeDoc/+merge/196301
> --
> https://code.launchpad.net/~kalikiana/u1db-qt/removeDoc/+merge/196301
> Your team U1DB Qt developers is requested to review the proposed merge of
> lp:~kalikiana/u1db-qt/removeDoc into lp:u1db-qt.
>
> === modified file 'src/database.cpp'
> --- src/database.cpp 2013-08-14 12:07:11 +0000
> +++ src/database.cpp 2013-11-22 15:24:45 +0000
> @@ -658,6 +658,15 @@
> }
>
> /*!
> + Removes the document identified by \a docId.
> + */
> +void
> +Database::removeDoc(const QString& docId)
> +{
> + putDoc(QString(), docId);
> +}
> +
> +/*!
> * \brief Database::resetModel
> *
> * Resets the Database model.
>
> === modified file 'src/database.h'
> --- src/database.h 2013-08-12 15:28:13 +0000
> +++ src/database.h 2013-11-22 15:24:45 +0000
> @@ -49,6 +49,7 @@
> QString getDocumentContents(const QString& docId);
> QVariant getDocUnchecked(const QString& docId) const;
> Q_INVOKABLE QString putDoc(QVariant newDoc, const QString&
> docID=QString());
> + Q_INVOKABLE void removeDoc(const QString& docID);
> Q_INVOKABLE QList<QString> listDocs();
> Q_INVOKABLE QString lastError();
> Q_INVOKABLE QString putIndex(const QString& index_name, QStringList
> expressions);
>
> === modified file 'tests/tst_query.qml'
> --- tests/tst_query.qml 2013-08-27 10:49:26 +0000
> +++ tests/tst_query.qml 2013-11-22 15:24:45 +0000
> @@ -182,7 +182,7 @@
> function test_4_delete () {
> compare(defaultPhone.documents, ['1', '_', 'a'], 'uno')
> // Deleted aka empty documents should not be returned
> - gents.putDoc('', '_')
> + gents.removeDoc('_')
> compare(defaultPhone.documents, ['1', 'a'], 'dos')
> }
>
>
>
> --
> Mailing list: https://launchpad.net/~uonedb-qt
> Post to : <email address hidden>
> Unsubscribe : https://launchpad.net/~uonedb-qt
> More help : https://help.launchpad.net/ListHelp
>
>

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:108
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/22/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/22
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/22
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/22
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/24
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-trusty-amd64-ci/2
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-trusty-armhf-ci/2

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/u1db-qt-ci/22/rebuild

review: Approve (continuous-integration)

[Cris Dywan (kalikiana)]

> Isn't it that documents should always be null or empty and not removed?
>
> If a document is null or empty it should be filtered out in Query or Index
> should it not? Those are the mechanisms for filtering results, it seems
> maybe the functionality should be there rather than Database, no?

There is no functional change in the proposed removeDoc, as you can see in the diff. But as the linked bug shows people are getting confused by the lack of API and it makes it also hard for me as a developer to distinguish between misunderstandings and real bugs.

[Stuart Langridge (sil)]

The function is called "delete_doc" in the reference implementation. Obviously names should fit the naming scheme of their implementation, but changing "delete_doc" in other implementations to "remove_doc" in the QML implementation seems to be confusing with no benefit.

[Stuart Langridge (sil)]

(https://one.ubuntu.com/developer/data/u1db/reference-implementation#u1db.Database.delete_doc)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:110
http://jenkins.qa.ubuntu.com/job/u1db-qt-ci/28/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-precise-amd64-ci/28
    FAILURE: http://jenkins.qa.ubuntu.com/job/u1db-qt-quantal-amd64-ci/28/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-raring-amd64-ci/28
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-saucy-amd64-ci/30
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-trusty-amd64-ci/8
    SUCCESS: http://jenkins.qa.ubuntu.com/job/u1db-qt-trusty-armhf-ci/8

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/u1db-qt-ci/28/rebuild

review: Needs Fixing (continuous-integration)

[Benjamin Zeller (zeller-benjamin)]

Compiles, tests work, changes look fine, good to go

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/database.cpp'
--- src/database.cpp	2013-08-14 12:07:11 +0000
+++ src/database.cpp	2014-01-24 11:14:39 +0000
@@ -658,6 +658,15 @@
 }
 
 /*!
+    Deletes the document identified by \a docId.
+ */
+void
+Database::deleteDoc(const QString& docId)
+{
+    putDoc(QString(), docId);
+}
+
+/*!
  * \brief Database::resetModel
  *
  * Resets the Database model.

=== modified file 'src/database.h'
--- src/database.h	2013-08-12 15:28:13 +0000
+++ src/database.h	2014-01-24 11:14:39 +0000
@@ -49,6 +49,7 @@
     QString getDocumentContents(const QString& docId);
     QVariant getDocUnchecked(const QString& docId) const;
     Q_INVOKABLE QString putDoc(QVariant newDoc, const QString& docID=QString());
+    Q_INVOKABLE void deleteDoc(const QString& docID);
     Q_INVOKABLE QList<QString> listDocs();
     Q_INVOKABLE QString lastError();
     Q_INVOKABLE QString putIndex(const QString& index_name, QStringList expressions);

=== modified file 'tests/tst_query.qml'
--- tests/tst_query.qml	2013-08-27 10:49:26 +0000
+++ tests/tst_query.qml	2014-01-24 11:14:39 +0000
@@ -182,7 +182,7 @@
     function test_4_delete () {
         compare(defaultPhone.documents, ['1', '_', 'a'], 'uno')
         // Deleted aka empty documents should not be returned
-        gents.putDoc('', '_')
+        gents.deleteDoc('_')
         compare(defaultPhone.documents, ['1', 'a'], 'dos')
     }