[Differential] [Commented On] D4537: EditorConfig support

[Grzegorz Szymaszek]

gszymaszek added a comment.


  In https://phabricator.kde.org/D4537#87411, @cullmann wrote:
  
  > Do you have commit rights? If not, paste your mail address and full name 
and I can push your change with you as author.
  
  
  No, I don’t have. Grzegorz Szymaszek, `gszymas...@short.pl`.

REPOSITORY
  R39 KTextEditor

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

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

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

[Differential] [Commented On] D4537: EditorConfig support

[Dominik Haumann]

dhaumann added a comment.


  Patch looks already pretty good, I think we're soon there.

INLINE COMMENTS

> gszymaszek wrote in editorconfig.cpp:23
> Is it OK to initialize `m_handle` in the constructor? If so, is `m_handle(0)` 
> necessary?

Yes, this is good now.

> editorconfig.cpp:34-56
> +bool EditorConfig::checkBoolValue(QString val, bool *result)
> +{
> +val = val.trimmed().toLower();
> +static const QStringList trueValues = QStringList() << 
> QStringLiteral("1") << QStringLiteral("on") << QStringLiteral("true");
> +if (trueValues.contains(val)) {
> +*result = true;
> +return true;

Can you move this into an unnamed namespace at the beginning of the file?
Then, you can move the API documentation from the header file to the cpp file 
as well (and remove the static functions from the private section).

> editorconfig.cpp:80-82
> +const char *key, *value;
> +key = nullptr;
> +value = nullptr;

Please only one variable per line:
const char *key = nullptr;
const char *value = nullptr;

> editorconfig.cpp:84-85
> +
> +// their Qt counterparts, for comparisons
> +QLatin1String keyString, valueString;
> +

Please declare variables locally, i.e. move down, see below.

> editorconfig.cpp:105-106
> +
> +keyString = QLatin1String(key);
> +valueString = QLatin1String(value);
> +

const QLatin1String keyString = ...
const QLatin1String valueString = ...

REPOSITORY
  R39 KTextEditor

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

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

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

[Differential] [Commented On] D4537: EditorConfig support

[Grzegorz Szymaszek]

gszymaszek added a comment.


  Yes, it was good when actual parsing was delegated to a separate function, 
but we’ve decided to merge all parsing-related functions.

REPOSITORY
  R39 KTextEditor

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

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

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

[Differential] [Commented On] D4537: EditorConfig support

[Christoph Cullmann]

cullmann added a comment.


  Ok, then I would opt for removing the comment.
  
  Beside that, I think in some places early outs would be preferable to 
nesting, e.g. like
  
  if (code != 0) {
  
if (code == EDITORCONFIG_PARSE_MEMORY_ERROR) {
  
  ​qCDebug(LOG_KTE) << "Failed to parse .editorconfig, memory error 
occurred";
  
else
  
  ​qCDebug(LOG_KTE) << "Failed to parse .editorconfig, unknown error";
  
return code;
  
  }
  
  then the rest non-indented

REPOSITORY
  R39 KTextEditor

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

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

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

[Differential] [Commented On] D4537: EditorConfig support

[Grzegorz Szymaszek]

gszymaszek added a comment.


  In https://phabricator.kde.org/D4537#86356, @cullmann wrote:
  
  > On the other side, I see no real copyrightable material beside you are 
using the editor config API.
  
  
  As I’ve written, I used Builder’s source to //learn// that (1) I have to call 
`editorconfig_handle_init` and `editorconfig_parse` at the beginning, (2) I 
have to call `editorconfig_handle_get_name_value` to read EditorConfig’s 
results (and `editorconfig_handle_get_name_value_count` to make reading easier) 
and (3) I have to call `editorconfig_handle_destroy` to “close connection”. 
Implementation details come from API docs and my imagination.
  
  Maybe it isn’t a good idea to include that comment about Builder in this 
patch — the code isn’t a “port” (or copy) of Builder’s solution to KTextEditor, 
while that’s what such comment may wrongly suggest.

REPOSITORY
  R39 KTextEditor

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

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

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

[Differential] [Commented On] D4537: EditorConfig support

[Christoph Cullmann]

cullmann added a comment.


  The problem with
  
  // reading .editorconfig files is loosely based on gnome-builder’s
  // libide/editorconfig/editorconfig-glib.c
  
  is that that code is GPL.
  
  On the other side, I see no real copyrightable material beside you are using 
the editor config API.

REPOSITORY
  R39 KTextEditor

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

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

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

[Differential] [Commented On] D4537: EditorConfig support

[Grzegorz Szymaszek]

gszymaszek added a comment.


  In https://phabricator.kde.org/D4537#85846, @dhaumann wrote:
  
  > Btw, did you copy some code from another project? If so, we need to be 
careful, since KTextEditor is LGPLv+2.
  
  
  I’ve used mentioned gnome-builder’s file 

 as a guide on how to deal with editorconfig-core-c (which functions should be 
used as a minimum to read .editorconfig file), because I didn’t find such guide 
in their API docs .

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

[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

[Differential] [Commented On] D4537: EditorConfig support

[Dominik Haumann]

dhaumann added a comment.


  If you want, you can also just copy or duplicate these functions, and we 
later look how to merge them again in a sane way.

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

[Differential] [Commented On] D4537: EditorConfig support

[Grzegorz Szymaszek]

gszymaszek added a comment.


  In the new `EditorConfig` class I need to use `DocumentPrivate`’s 
`checkBoolValue` and `checkIntValue` methods, but they’re declared private. I 
think it should be OK to make them public as they’re static anyway, but I’d 
like to hear what’s your opinion. Setting `EditorConfig` as `DocumentPrivate`’s 
friend didn’t help.

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

[Differential] [Commented On] D4537: EditorConfig support

[Dominik Haumann]

dhaumann added a comment.


  Yes, please create a separate review request for the extra-cmake-modules 
repository. There, you will also get a good review by developers who know cmake 
very well (which I do not), so in the end we will also have a very god 
Findeditorconfig cmake module. Thanks!

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

[Differential] [Commented On] D4537: EditorConfig support

[Grzegorz Szymaszek]

gszymaszek added a comment.


  Thanks for your comments. I’ve managed to make editorconfig optional using 
the mentioned `FindEditorConfig.cmake` module (I had to change its name to 
`Findeditorconfig.cmake`). Should I create a new diff in ECM repository to add 
this file?
  Now I’m going to create a separate class for EditorConfig parsing.

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

[Differential] [Commented On] D4537: EditorConfig support

[Dominik Haumann]

dhaumann added a comment.


  By the way: The find module for libgit2 is here: 
https://github.com/KDE/extra-cmake-modules/blob/master/find-modules/FindLibGit2.cmake
  Maybe you can add one (if none exists already) for the EditorConfig lib?

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

[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