add cpp interface and unit tests by xqp · Pull Request #36 · Axosoft/nsfw

xqp · 2017-09-25T22:52:23Z

this is a PR for the future work of #34

what we have worked on?

With this changes we can use and test the library with c++.

After the installation of cmake you can run the following command
cmake . && make -j

reneme · 2017-09-26T13:20:15Z

includes/Queue.h

  Event(const EventType type, const std::string &directory, const std::string &fileA, const std::string &fileB) :
-      type(type), directory(directory), fileA(fileA), fileB(fileB) {}
+      type(type), directory(directory), fileA(fileA), fileB(fileB) {
+    timePoint = std::chrono::high_resolution_clock::now();


rename timePoint to the more idiomatic name timestamp, please

with forgiveness I think it is a time point because the std::chrono libraries doesn't know any 'timestamps' just a lot of time points

see: http://en.cppreference.com/w/cpp/chrono/time_point
"Class template std::chrono::time_point represents a point in time. It is implemented as if it stores a value of type Duration indicating the time interval from the start of the Clock's epoch."

if we rename this as "timestamp" there is confusion with some known timestamps

reneme · 2017-09-26T13:28:26Z

includes/transforms/AbstractTransform.h

+        return transform(std::move(vecEvents));
+    }
+
+    virtual VecEvents transform(VecEvents vecEvents) = 0;


style: NSFW uses two-spaced tabs.

reneme · 2017-09-26T13:30:47Z

includes/transforms/ExcludeDirectories.h

+                                 [this](std::unique_ptr<Event> &event){
+      std::regex self_regex(regex,
+                            std::regex_constants::ECMAScript
+                          | std::regex_constants::icase);


not sure about the detailed semantics here. Though, my feeling is that you should set up self_regex outside the lambda and capture it. Otherwise it would recompile the regex several times, no?

reneme · 2017-09-26T13:32:47Z

includes/transforms/ExcludeDirectories.h

+  ExcludeDirectories(const std::string &regex)
+    : regex(regex) {}
+
+  VecEvents transform(VecEvents vecEvents) {


please implement that in a *.cpp file.

reneme · 2017-09-26T13:33:00Z

includes/transforms/ExcludeDirectories.h

+    return vecEvents;
+  }
+
+  virtual ~ExcludeDirectories() {}


reneme · 2017-09-26T13:52:38Z

test/u_ExcludeDirectories.cpp

+    std::string regex = "/bar$";
+    ExcludeDirectories exDir(regex);
+
+    SECTION("directory name does match") {


does NOT match, no?

reneme · 2017-09-26T13:53:31Z

test/u_ExcludeDirectories.cpp

+      auto transformed = exDir(std::move(vec));
+      REQUIRE(transformed->size() == 0);
+    }
+  }


another test with both matching and non-matching dir names would be great.

reneme · 2017-09-26T13:53:51Z

test/u_ExcludeFiles.cpp

+    std::string regex = "~[a-zA-Z_][a-zA-Z_0-9]*\\.tmp";
+    ExcludeFiles exFiles(regex);
+
+    SECTION("file name does match") {


does NOT match, again?

reneme · 2017-09-26T13:55:38Z

test/u_FileWatcher.cpp

+
+#include "test_helper.h"
+
+std::string getDirectoryFromFile(const std::string &path) {


should be static (i.e. not part of the public interface of this compilation unit)

reneme · 2017-09-26T13:57:03Z

test/u_FileWatcher.cpp

+  std::string executionPath(getDirectoryFromFile(programmName));
+  TestFileSystemAdapter testWatcher(executionPath,
+                                    std::chrono::milliseconds(10));
+  auto comparation = [](const Event &lhs, const Event &rhs) {


reneme · 2017-09-27T07:24:43Z

includes/Listener.hpp

+    void notify(Args&&... args) {
+        std::lock_guard<std::mutex> lock(mListenersMutex);
+        for(const auto &func : mListeners) {
+            func.second(std::move(std::forward<Args>(args)...));


Retrofitting this into our private code base I noticed that the template parameter unpacking is wrong here. It doesn't work for multi-argument Listener<> callbacks. Needs to look like this:

// move closing paren of std::move _before_ param unpacking func.second(std::move(std::forward<Args>(args))...);

reneme · 2017-09-27T07:25:03Z

includes/Listener.hpp

+
+template <class CallbackType>
+class Listener
+{


two-spaced tabs, please.

reneme · 2017-09-27T07:26:40Z

src/transforms/ExcludeDirectories.cpp

+}
+
+VecEvents ExcludeDirectories::transform(VecEvents vecEvents) {
+    std::regex self_regex(mRegex,


two-spaced tabs, please.

reneme · 2017-10-04T08:05:19Z

@implausible @maxkorp Any thoughts on that? :-)

implausible · 2017-11-01T20:39:00Z

Hello again! @reneme I will get to this over the weekend.

add namespaces change the includes to a relativpath independent path

implausible

Test framework should run on travis/appveyor.

xqp · 2017-11-14T11:05:53Z

okay :) the test framework run on travis and appveyor..

except the mac implementation..
the macos implementation creates a ADD and Delete action for the std::rename() command o.O

implausible · 2017-11-28T18:14:18Z

That's a little strange. There's also some inconsistency on windows as well.

Patrik Fiedler and others added 13 commits September 15, 2017 15:49

add transform and filter functionality for nsfw

ebac2b3

add some tests for the transform files

ec5013b

add testcases for the FileWatcher implementation

7f83be1

fix windows compile errors

9b06a54

add transform and filter functionality for nsfw

08a55c9

add some tests for the transform files

31090b2

add testcases for the FileWatcher implementation

56e485c

fix windows compile errors

a358c03

modify cmake and add ctest

d9bb3c9

repair code style

f0d00a5

merge conflicts

250529b

delete comment in cmakelists

0e90a8a

add virtual destructors

2a3f7b0

reneme suggested changes Sep 26, 2017

View reviewed changes

review issues

1f437b0

reneme suggested changes Sep 27, 2017

View reviewed changes

Patrik Fiedler added 2 commits September 27, 2017 13:34

review issues

07fe872

indentation

1eda018

Patrik Fiedler and others added 7 commits November 3, 2017 09:36

change a lot of thing to use this as a library

f031693

install the project with cmake

c4c31c3

add include guards

ae40db2

add namespaces for nsfw

57fbfcf

add namespaces in whole project and every supported platform

ab64839

change the js entry class to the library

b2aebce

add namespaces change the includes to a relativpath independent path

FIX: compilation on macOS

f8bab16

implausible suggested changes Nov 5, 2017

View reviewed changes

Patrik Fiedler added 2 commits November 7, 2017 13:50

fix Filewatcher Test

daba5e8

integrate cmake and ctest to travis and appveyor

020c1fc

Patrik Fiedler and others added 4 commits January 24, 2018 16:19

Merge branch 'master' into feature/add_cpp_interface_for_nsfw

c9f8cb8

update Catch to Catch2 2.1.0

d931ef3

disable bogus test on macOS now

288c317

fix mutex locking

4f1c5ec


		#include "test_helper.h"

		std::string getDirectoryFromFile(const std::string &path) {

Conversation

xqp commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reneme commented Oct 4, 2017

Uh oh!

implausible commented Nov 1, 2017

Uh oh!

implausible left a comment

Choose a reason for hiding this comment

Uh oh!

xqp commented Nov 14, 2017

Uh oh!

implausible commented Nov 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xqp commented Sep 25, 2017 •

edited

Loading