From b7fb24837ae59582b3e6baf5a967b1848dfde79e Mon Sep 17 00:00:00 2001
From: Adriaan de Groot <groot@kde.org>
Date: Wed, 10 Jan 2018 11:01:39 -0500
Subject: [PATCH] [shellprocess] Improve CommandList

 - Also allow a single string instead of a list
 - Add count() method to CommandList
 - Drop over-engineering, add more logging
 - Expand tests with some more examples
---
 src/modules/shellprocess/CommandList.cpp | 51 +++++++++++-------------
 src/modules/shellprocess/CommandList.h   | 11 ++---
 src/modules/shellprocess/Tests.cpp       | 50 ++++++++++++++++++++++-
 src/modules/shellprocess/Tests.h         |  7 +++-
 4 files changed, 83 insertions(+), 36 deletions(-)

diff --git a/src/modules/shellprocess/CommandList.cpp b/src/modules/shellprocess/CommandList.cpp
index 261ff9429..1e7f4887a 100644
--- a/src/modules/shellprocess/CommandList.cpp
+++ b/src/modules/shellprocess/CommandList.cpp
@@ -18,53 +18,50 @@
 
 #include "CommandList.h"
 
+#include "utils/Logger.h"
+
 #include <QVariantList>
 
-class CommandList::Private
-{
-public:
-    Private( const QVariantList& l );
-    ~Private();
-} ;
-
-CommandList::Private::Private(const QVariantList& l)
+static QStringList get_variant_stringlist(const QVariantList& l)
 {
+    QStringList retl;
+    unsigned int c = 0;
+    for ( const auto& v : l )
+    {
+        if ( v.type() == QVariant::String )
+            retl.append( v.toString() );
+        else
+            cDebug() << "WARNING Bad CommandList element" << c << v.type() << v;
+        ++c;
+    }
+    return retl;
 }
 
-CommandList::Private::~Private()
-{
-}
-
-
-
 CommandList::CommandList()
-    : m_d( nullptr )
 {
 }
 
 CommandList::CommandList::CommandList(const QVariant& v)
     : CommandList()
 {
-    if ( ( v.type() == QVariant::List ) )
+    if ( v.type() == QVariant::List )
     {
         const auto v_list = v.toList();
         if ( v_list.count() )
         {
-            m_d = new Private( v_list );
+            append( get_variant_stringlist( v_list ) );
         }
+        else
+            cDebug() << "WARNING: Empty CommandList";
     }
+    else if ( v.type() == QVariant::String )
+    {
+        append( v.toString() );
+    }
+    else
+        cDebug() << "WARNING: CommandList does not understand variant" << v.type();
 }
 
 CommandList::~CommandList()
 {
-    delete m_d;
-    m_d = nullptr;  // TODO: UniquePtr
-}
-
-bool CommandList::isEmpty() const
-{
-    if ( !m_d )
-        return true;
-
-    return false;  // FIXME: actually count things
 }
diff --git a/src/modules/shellprocess/CommandList.h b/src/modules/shellprocess/CommandList.h
index ed6b69b00..3b8cdcc93 100644
--- a/src/modules/shellprocess/CommandList.h
+++ b/src/modules/shellprocess/CommandList.h
@@ -19,21 +19,18 @@
 #ifndef COMMANDLIST_H
 #define COMMANDLIST_H
 
+#include <QStringList>
 #include <QVariant>
 
-class CommandList
+class CommandList : protected QStringList
 {
-    class Private;
-
 public:
     CommandList();
     CommandList(const QVariant& v);
     ~CommandList();
 
-    bool isEmpty() const;
-
-private:
-    Private *m_d;
+    using QStringList::isEmpty;
+    using QStringList::count;
 } ;
 
 #endif // COMMANDLIST_H
diff --git a/src/modules/shellprocess/Tests.cpp b/src/modules/shellprocess/Tests.cpp
index f30619c1f..13a611136 100644
--- a/src/modules/shellprocess/Tests.cpp
+++ b/src/modules/shellprocess/Tests.cpp
@@ -44,7 +44,7 @@ ShellProcessTests::initTestCase()
 }
 
 void
-ShellProcessTests::testProcessList()
+ShellProcessTests::testProcessListSampleConfig()
 {
     YAML::Node doc;
 
@@ -62,4 +62,52 @@ ShellProcessTests::testProcessList()
     CommandList cl(
         CalamaresUtils::yamlMapToVariant( doc ).toMap().value( "script" ) );
     QVERIFY( !cl.isEmpty() );
+    QCOMPARE( cl.count(), 2 );
+}
+
+void ShellProcessTests::testProcessListFromList()
+{
+    YAML::Node doc = YAML::Load( R"(---
+script:
+    - "ls /tmp"
+    - "ls /nonexistent"
+    - "/bin/false"
+)" );
+    CommandList cl(
+        CalamaresUtils::yamlMapToVariant( doc ).toMap().value( "script" ) );
+    QVERIFY( !cl.isEmpty() );
+    QCOMPARE( cl.count(), 3 );
+
+    // Contains 1 bad element
+    doc = YAML::Load( R"(---
+script:
+    - "ls /tmp"
+    - false
+    - "ls /nonexistent"
+)" );
+    CommandList cl1(
+        CalamaresUtils::yamlMapToVariant( doc ).toMap().value( "script" ) );
+    QVERIFY( !cl1.isEmpty() );
+    QCOMPARE( cl1.count(), 2 );  // One element ignored
+}
+
+void ShellProcessTests::testProcessListFromString()
+{
+    YAML::Node doc = YAML::Load( R"(---
+script: "ls /tmp"
+)" );
+    CommandList cl(
+        CalamaresUtils::yamlMapToVariant( doc ).toMap().value( "script" ) );
+    QVERIFY( !cl.isEmpty() );
+    QCOMPARE( cl.count(), 1 );
+
+    // Not a string
+    doc = YAML::Load( R"(---
+script: false
+)" );
+    CommandList cl1(
+        CalamaresUtils::yamlMapToVariant( doc ).toMap().value( "script" ) );
+    QVERIFY( cl1.isEmpty() );
+    QCOMPARE( cl1.count(), 0 );
+
 }
diff --git a/src/modules/shellprocess/Tests.h b/src/modules/shellprocess/Tests.h
index 82f93afe6..fa29efeb2 100644
--- a/src/modules/shellprocess/Tests.h
+++ b/src/modules/shellprocess/Tests.h
@@ -30,7 +30,12 @@ public:
 
 private Q_SLOTS:
     void initTestCase();
-    void testProcessList();
+    // Check the sample config file is processed correctly
+    void testProcessListSampleConfig();
+    // Create from a YAML list
+    void testProcessListFromList();
+    // Create from a simple YAML string
+    void testProcessListFromString();
 };
 
 #endif