[Differential] [Commented On] D4537: EditorConfig support

[Dominik Haumann]

dhaumann added a comment.


  The patch looks already much better. I have added comments below.
  
  Btw, did you copy some code from another project? If so, we need to be careful, since KTextEditor is LGPLv+2.

INLINE COMMENTS

> CMakeLists.txt:54
> +# EditorConfig support
> +# TODO: find oldest working version (0.12.0 was released 2014-10-12)
> +find_package(editorconfig "0.12.0")

I think you can just write "# EditorConfig support (0.12.0 was released 2014-10-12)"
There is no need for looking for even older versions I think.

> CMakeLists.txt:89
>  document/katebuffer.cpp
> +document/editorconfig.cpp
>  

Since the editorconfig is optional, I would make compiling this optional as well:

  if (EDITORCONFIG_FOUND) # optional support for EditorConfig
    set(ktexteditor_LIB_SRCS
      ${ktexteditor_LIB_SRCS}
      document/editorconfig.cpp
    )
  endif()

This way, it's truly optional :-)

> editorconfig.cpp:23
> +
> +EditorConfig::EditorConfig(KTextEditor::DocumentPrivate *document)
> +{

Better is to write:

  EditorConfig::EditorConfig(KTextEditor::DocumentPrivate *document)
      : m_document(document)
      , m_handle(0)
  {
      // ...
  }

> editorconfig.cpp:160
> +{
> +    int code = editorconfig_parse(m_document->url().toLocalFile().toStdString().c_str(), m_handle);
> +

const int code = ...

Please use const *whenever possible* :-)

> editorconfig.h:56-57
> +    static bool checkIntValue(QString value, int *result);
> +    void interpret();
> +    void interpretLine(const char *key, const char *value);
> +

I would prefer to merge these into parse(), then we also do not need all the member variables anymore.

Rule of thumb: Each member variable adds more states to a class. The more states, the less easy it is to understand when a member changes, since it can be used in any function in the class. In contrast, if variables are local to a function, this reduces states of the class. Therefore, it is typically much easier to understand what the class is doing.

> katedocument.cpp:2589-2590
> +    // file, if such is provided
> +    EditorConfig *editorConfig = new EditorConfig(this);
> +    editorConfig->parse();
> +#endif

No need to put this on the heap, especially since we are also missing a delete here (memory leak). Just write:

  EditorConfig editorConfig(this);
  editorConfig.parse();

> katedocument.h:29-31
> +#ifdef EDITORCONFIG_FOUND
> +#include "editorconfig.h"
> +#endif

It is enough to move this to the .cpp file, since it is completely unused in the header or by any other files.

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/20170213/0dd5d64f/attachment-0001.html>