dhaumann added a comment.

  Cool, I have some comments, though :-)
  
  1. Optional Dependency
  
    As you yourself note, please make this an optional dependency: Best is if 
we could use find_package(editorconfig) or so to make sure it is consistent how 
we typically also add dependencies. For instance, we use find_package(LibGit2 
"0.22.0") in ktexteditor/CMakeLists.txt for the optional dependency on libgit2. 
Then, later we use #ifdef LIBGIT2_FOUND to optionally use the libgit2 library. 
We should do the same with editorconfig.
  
  Does a find_package module exist here?
  
  2. Separate Class
  
    I would prefer to have a standalone class that handles the editorconfig: 
I.e. a class EditorConfig in a separate cpp/h file. We could pass the Document 
doc in the EditorConfig(doc) constructor and then let the EditorConfig class do 
the work. This also has the advantage that we can add unit tests for the 
EditorConfig class, which is not so easy otherwise.
  
  Could you provide an updated patch, or do you need help somewhere?

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D4537

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: gszymaszek, #ktexteditor
Cc: dhaumann, kwrite-devel, #frameworks

Reply via email to