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
- Reviewers
cullmann - Group Reviewers
KTextEditor - Commits
- R39:f9f133b6ac72: EditorConfig support
Diff Detail
- Repository
- R39 KTextEditor
- Lint
Lint Skipped - Unit
Unit Tests Skipped
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?
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.
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.
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!
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.
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.
Moved EditorConfig-related logic into a separate class and made it optional (no longer a KTextEditor dependency).
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 | 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. |
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? |
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.
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.
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.
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
Yes, it was good when actual parsing was delegated to a separate function, but we’ve decided to merge all parsing-related functions.
Patch looks already pretty good, I think we're soon there.
src/document/editorconfig.cpp | ||
---|---|---|
24 | Yes, this is good now. | |
34–56 ↗ | (On Diff #11349) | Can you move this into an unnamed namespace at the beginning of the file? |
80–82 ↗ | (On Diff #11349) | Please only one variable per line: |
84–85 ↗ | (On Diff #11349) | Please declare variables locally, i.e. move down, see below. |
105–106 ↗ | (On Diff #11349) | const QLatin1String keyString = ... |
Pulled check-variable-functions out of EditorConfig class, moved some variable definitions into main loop of parser.
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