[Differential] [Commented On] D4537: EditorConfig support

[Dominik Haumann]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170212/13ad814d/attachment.html>