This diff enables support for reading configurations from .editorconfig files, as an alternative to already supported .kateconfig. EditorConfig is supported in many other editors and IDEs. In case of KTextEditor it’s done using editorconfig-core-c library that I’ve added to CMakeLists.txt and katedocument.h. A disadvantage of my approach is that this library became a KTextEditor’s dependency, maybe it should be made optional?
Fixes bug 330843.
Details
Details
- Reviewers
cullmann - Group Reviewers
KTextEditor - Commits
- R39:f9f133b6ac72: EditorConfig support
Diff Detail
Diff Detail
- Repository
- R39 KTextEditor
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Cool, I have some comments, though :-)
- 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?
- 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?
Comment Actions
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?
EDIT: A FindEditorConfig.cmake module already exists:
https://github.com/editorconfig/editorconfig-core-lua/tree/master/cmake/Modules
This should be part of the extra-cmake-modules, though.
Comment Actions
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.
Comment Actions
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!
Comment Actions
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.
Comment Actions
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.
Comment Actions
Moved EditorConfig-related logic into a separate class and made it optional (no longer a KTextEditor dependency).
Comment Actions
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.
| CMakeLists.txt | ||
|---|---|---|
| 54 ↗ | (On Diff #11262) | I think you can just write "# EditorConfig support (0.12.0 was released 2014-10-12)" |
| src/CMakeLists.txt | ||
| 89 | 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 :-) | |
| src/document/editorconfig.cpp | ||
| 24 | Better is to write: EditorConfig::EditorConfig(KTextEditor::DocumentPrivate *document)
: m_document(document)
, m_handle(0)
{
// ...
} | |
| 161 | const int code = ... Please use const *whenever possible* :-) | |
| src/document/editorconfig.h | ||
| 57–58 | 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. | |
| src/document/katedocument.cpp | ||
| 2589–2590 | 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(); | |
| src/document/katedocument.h | ||
| 29–31 ↗ | (On Diff #11262) | It is enough to move this to the .cpp file, since it is completely unused in the header or by any other files. |
Comment Actions
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.
| src/document/editorconfig.cpp | ||
|---|---|---|
| 24 | Is it OK to initialize m_handle in the constructor? If so, is m_handle(0) necessary? | |
Comment Actions
Fixed src/CMakeLists.txt to make EditorConfig support truly optional, removed interpret and interpretLine functions (so parsing is back in parse), simplified EditorConfig class constructor, fixed memory leak in DocumentPrivate.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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
Comment Actions
Yes, it was good when actual parsing was delegated to a separate function, but we’ve decided to merge all parsing-related functions.
Comment Actions
Patch looks already pretty good, I think we're soon there.
| src/document/editorconfig.cpp | ||
|---|---|---|
| 24 | Yes, this is good now. | |
| 35–57 | Can you move this into an unnamed namespace at the beginning of the file? | |
| 81–83 | Please only one variable per line: | |
| 85–86 | Please declare variables locally, i.e. move down, see below. | |
| 106–107 | const QLatin1String keyString = ... | |
Comment Actions
Pulled check-variable-functions out of EditorConfig class, moved some variable definitions into main loop of parser.
Comment Actions
Hi, please adjust your patch to the EditorConfig find module commited to ktexteditor.git.
Then this can land.
Do you have commit rights? If not, paste your mail address and full name and I can push your change with you as author.
commit with find module: https://commits.kde.org/ktexteditor/6fa17cdccc064b68ea7cba058eff9cac6332e6b9