diff options
Diffstat (limited to 'CONTRIBUTING.md')
| -rw-r--r-- | CONTRIBUTING.md | 270 |
1 files changed, 145 insertions, 125 deletions
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ba68d226..fd604621 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -142,120 +142,20 @@ The code should strive to be DRY (don't repeat yourself), clear, and obviously correct (i.e. buildable). Some technical debt is inevitable, just don't bankrupt us with it. Refactoring is welcome. -### Generated C++ code for CS API -The code in lib/csapi, lib/identity and lib/application-service, although -it resides in Git, is actually generated from (a soft fork of) the official -Matrix Swagger/OpenAPI definition files. Do not edit C++ files -in these directories by hand! - -Now, if you're unhappy with something in there and want to improve the code, -you have to understand the way these files are produced and setup -some additional tooling. The shortest possible procedure resembling -the below text can be found in .travis.yml (our Travis CI configuration -actually regenerates those files upon every build). As described below, -there is a handy build target for CMake; patches with a similar target -for qmake are (you guessed it) very welcome. - -#### Why generate the code at all? -Because before both original authors of libQuotient had to do monkey business of writing boilerplate code, with the same patterns, types etc., literally, for every single API endpoint, and one of the authors got fed up with it at some point in time. By then about 15 job classes were written; the entire API counts about 100 endpoints. Besides, the existing jobs had to be updated according to changes in CS API that have been, and will keep, coming. Other considerations can be found in [this talk about API description languages that briefly touches on GTAD](https://youtu.be/W5TmRozH-rg). - -#### Prerequisites for CS API code generation -1. Get the source code of GTAD and its dependencies, e.g. using the command: - `git clone --recursive https://github.com/KitsuneRal/gtad.git` -2. Build GTAD: in the source code directory, do `cmake . && cmake --build .` - (you might need to pass `-DCMAKE_PREFIX_PATH=<path to Qt>`, - similar to libQuotient itself). -3. Get the Matrix CS API definitions that are included in the matrix-doc repo: - `git clone https://github.com/quotient-im/matrix-doc.git` - (quotient-im/matrix-doc is a fork that's known to produce working code; - you may want to use your own fork if you wish to alter something in the API). -4. If you plan to submit a PR or just would like the generated code to be - formatted, you should either ensure you have clang-format (version 6 at least) - in your PATH or pass the _absolute_ path to it by adding - `-DCLANG_FORMAT=<absolute path>` to the CMake invocation below. - -#### Generating CS API contents -1. Pass additional configuration to CMake when configuring libQuotient: - `-DMATRIX_DOC_PATH=<path you your matrix-doc repo> -DGTAD_PATH=<path to gtad binary (not the repo!)>`. - If everything's right, these two CMake variables will be mentioned in - CMake output and will trigger configuration of an additional build target, - see the next step. -2. Generate the code: `cmake --build <your build dir> --target update-api`; - if you use CMake with GNU Make, you can just do `make update-api` instead. - Building this target will create (overwriting without warning) `.h` and `.cpp` - files in `lib/csapi`, `lib/identity`, `lib/application-service` for all - YAML files it can find in `matrix-doc/api/client-server` and other files - in `matrix-doc/api` these depend on. -3. Re-run CMake so that the build system knows about new files, if there are any. - -#### Changing generated code -See the more detailed description of what GTAD is and how it works in the documentation on GTAD in its source repo. Only parts specific for libQuotient are described here. - -GTAD uses the following three kinds of sources: -1. OpenAPI files. Each file is treated as a separate source (because this is how GTAD works now). -2. A configuration file, in our case it's lib/csapi/gtad.yaml - this one is common for the whole API. -3. Source code template files: lib/csapi/{{base}}.*.mustache - also common. - -The mustache files have a templated (not in C++ sense) definition of a network -job, deriving from BaseJob; each job class is prepended, if necessary, with -data structure definitions used by this job. The look of those files is hideous -for a newcomer; and the only known highlighter that can handle the combination -of Mustache (originally a web templating language) and C++ is provided in CLion. -To slightly simplify things some more or less generic constructs are defined -in gtad.yaml (see its "mustache:" section). Adventurous souls that would like -to figure what's going on in these files should speak up in the Quotient room - -I (Kitsune) will be very glad to help you out. - -The types map in `gtad.yaml` is the central switchboard when it comes to matching OpenAPI types with C++ (and Qt) ones. It uses the following type attributes aside from pretty obvious "imports:": -* `avoidCopy` - this attribute defines whether a const ref should be used instead of a value. For basic types like int this is obviously unnecessary; but compound types like `QVector` should rather be taken by reference when possible. -* `moveOnly` - some types are not copyable at all and must be moved instead (an obvious example is anything "tainted" with a member of type `std::unique_ptr<>`). The template will use `T&&` instead of `T` or `const T&` to pass such types around. -* `useOmittable` - wrap types that have no value with "null" semantics (i.e. number types and custom-defined data structures) into a special `Omittable<>` template defined in `converters.h` - a substitute for `std::optional` from C++17 (we're still at C++14 yet). -* `omittedValue` - an alternative for `useOmittable`, just provide a value used for an omitted parameter. This is used for bool parameters which normally are considered false if omitted (or they have an explicit default value, passed in the "official" GTAD's `defaultValue` variable). -* `initializer` - this is a _partial_ (see GTAD and Mustache documentation for explanations but basically it's a variable that is a Mustache template itself) that specifies how exactly a default value should be passed to the parameter. E.g., the default value for a `QString` parameter is enclosed into `QStringLiteral`. - -Instead of relying on the event structure definition in the OpenAPI files, `gtad.yaml` uses pointers to libQuotient's event structures: `EventPtr`, `RoomEventPtr` and `StateEventPtr`. Respectively, arrays of events, when encountered in OpenAPI definitions, are converted to `Events`, `RoomEvents` and `StateEvents` containers. When there's no way to figure the type from the definition, an opaque `QJsonObject` is used, leaving the conversion to the library and/or client code. - -### Comments - -Whenever you add a new call to the library API that you expect to be used -from client code, you must supply a proper doc-comment along with the call. -Doxygen style is preferred; but JavaDoc is acceptable too. Some parts are -not documented at all; adding doc-comments to them is highly encouraged. - -Doc-comments for summaries should be separate from those details. Either of -the two following ways is fine, with considerable preference on the first one: -1. Use `///` for the summary comment and `/*! ... */` for details. -2. Use `\brief` (or `@brief`) for the summary, and follow with details after - an empty doc-comment line. You can use either of the delimiters in that case. - -In the code, the advice for commenting is as follows: -* Don't restate what's happening in the code unless it's not really obvious. - We assume the readers to have at least some command of C++ and Qt. If your - code is not obvious, consider rewriting it for clarity. -* Both C++ and Qt still come with their arcane features and dark corners, - and we don't want to limit anybody who'd feels they have a case for - variable templates, raw literals, or use `qAsConst` to avoid container - detachment. Use your experience to figure what might be less well-known to - readers and comment such cases (references to web pages, Quotient wiki etc. - are very much ok). -* Make sure to document not so much "what" but more "why" certain code is done - the way it is. In the worst case, the logic of the code can be - reverse-engineered; you rarely can reverse-engineer the line of reasoning and - the pitfalls avoided. - -### API conventions - -Calls, data structures and other symbols not intended for use by clients -should _not_ be exposed in (public) .h files, unless they are necessary -to declare other public symbols. In particular, this involves private members -(functions, typedefs, or variables) in public classes; use pimpl idiom to hide -implementation details as much as possible. `_impl` namespace is reserved for -definitions that should not be used by clients and are not covered by -API guarantees. - -Note: As of now, all header files of libQuotient are considered public; this may change eventually. - -### Code formatting +### Code style and formatting + +As of Quotient 0.6, the C++ standard for newly written code is C++17, with a few +restrictions, notably: +* standard library's _deduction guides_ cannot be used to lighten up syntax + in template instantiation, i.e. you have to still write + `std::array<int, 2> { 1, 2 }` instead of `std::array { 1, 2 }` or use helpers + like `std::make_pair` - once we move over to the later Apple toolchain, this + will be no more necessary. +* enumerators and slots cannot have `[[attributes]]` because moc of Qt 5.9 + chokes on them. This will be lifted when we move on to Qt 5.12 for the oldest + supported version. +* things from `std::filesystem` cannot be used yet until we push the oldest + required g++/libc to version 8. The code style is defined by `.clang-format`, and in general, all C++ files should follow it. Files with minor deviations from the defined style are still @@ -301,20 +201,68 @@ Additional considerations: [great article by Marc Mutz on Qt containers](https://marcmutz.wordpress.com/effective-qt/containers/) for details. +### API conventions + +Calls, data structures and other symbols not intended for use by clients +should _not_ be exposed in (public) .h files, unless they are necessary +to declare other public symbols. In particular, this involves private members +(functions, typedefs, or variables) in public classes; use pimpl idiom to hide +implementation details as much as possible. `_impl` namespace is reserved for +definitions that should not be used by clients and are not covered by +API guarantees. + +Note: As of now, all header files of libQuotient are considered public; this may change eventually. + +### Comments + +Whenever you add a new call to the library API that you expect to be used +from client code, you must supply a proper doc-comment along with the call. +Doxygen style is preferred; but JavaDoc is acceptable too. Some parts are +not documented at all; adding doc-comments to them is highly encouraged. + +Doc-comments for summaries should be separate from those details. Either of +the two following ways is fine, with considerable preference on the first one: +1. Use `///` for the summary comment and `/*! ... */` for details. +2. Use `\brief` (or `@brief`) for the summary, and follow with details after + an empty doc-comment line. You can use either of the delimiters in that case. + +In the code, the advice for commenting is as follows: +* Don't restate what's happening in the code unless it's not really obvious. + We assume the readers to have at least some command of C++ and Qt. If your + code is not obvious, consider rewriting it for clarity. +* Both C++ and Qt still come with their arcane features and dark corners, + and we don't want to limit anybody who'd feels they have a case for + variable templates, raw literals, or use `qAsConst` to avoid container + detachment. Use your experience to figure what might be less well-known to + readers and comment such cases (references to web pages, Quotient wiki etc. + are very much ok). +* Make sure to document not so much "what" but more "why" certain code is done + the way it is. In the worst case, the logic of the code can be + reverse-engineered; you rarely can reverse-engineer the line of reasoning and + the pitfalls avoided. + ### Automated tests There's no testing framework as of now; either Catch or Qt Test or both will be used eventually. -As a stopgap measure, qmc-example is used for automated functional testing. -Therefore, any significant addition to the library API should be accompanied -by a respective test in qmc-example. To add a test you should: -- Add a new private slot to the `QMCTest` class. -- Add to the beginning of the slot the line `running.push_back("Test name");`. -- Add test logic to the slot, using `QMC_CHECK` macro to assert the test outcome. ALL (even failing) branches should conclude with a QMC_CHECK invocation, unless you intend to have a "DID NOT FINISH" message in the logs under certain conditions. -- Call the slot from `QMCTest::startTests()`. - -`QMCTest` sets up some basic test fixture to help you with testing; notably by the moment `startTests()` is invoked you can rely on having a working connection in `c` member variable and a test room in `targetRoom` member variable. PRs to introduce a proper testing framework are very welcome (make sure to migrate tests from qmc-example though); shifting qmc-example to use Qt Test seems to be a particularly low-hanging fruit. +The `tests/` directory contains a command-line program, quotest, used for +automated functional testing. Any significant addition to the library API +should be accompanied by a respective test in quotest. To add a test you should: +- Add a new test to the `TestSuite` class (technically, each test is a private + slot and there are two macros, `TEST_DECL()` and `TEST_IMPL()`, that conceal + passing the testing handle in `thisTest` variable to the test method). +- Add test logic to the slot, using `FINISH_TEST` macro to assert the test + outcome and complete the test (`FINISH_TEST` contains `return`). ALL + (even failing) branches should conclude with a `FINISH_TEST` (or `FAIL_TEST` + that is a shortcut for a failing `FINISH_TEST`) invocation, unless you + intend to have a "DID NOT FINISH" message in the logs in certain conditions. + +The `TestManager` class sets up some basic test fixture to help you with testing; +notably, the tests can rely on having an initialised `Room` object for the test +room in `targetRoom` member variable. PRs to introduce a proper testing framework +are very welcome (make sure to migrate tests from quotest though). Note that +tests can go async, which is the biggest hurdle for Qt Test adoption. ### Security, privacy, and performance @@ -338,6 +286,79 @@ limit by passing the new value (in microseconds) in PROFILER_LOG_USECS to the compiler. In the future, this parameter will be made changeable at runtime _if_ needed. +### Generated C++ code for CS API +The code in lib/csapi, lib/identity and lib/application-service, although +it resides in Git, is actually generated from (a soft fork of) the official +Matrix Swagger/OpenAPI definition files. Do not edit C++ files +in these directories by hand! + +Now, if you're unhappy with something in there and want to improve the code, +you have to understand the way these files are produced and setup +some additional tooling. The shortest possible procedure resembling +the below text can be found in .travis.yml (our Travis CI configuration +actually regenerates those files upon every build). As described below, +there is a handy build target for CMake; patches with a similar target +for qmake are (you guessed it) very welcome. + +#### Why generate the code at all? +Because before both original authors of libQuotient had to do monkey business of writing boilerplate code, with the same patterns, types etc., literally, for every single API endpoint, and one of the authors got fed up with it at some point in time. By then about 15 job classes were written; the entire API counts about 100 endpoints. Besides, the existing jobs had to be updated according to changes in CS API that have been, and will keep, coming. Other considerations can be found in [this talk about API description languages that briefly touches on GTAD](https://youtu.be/W5TmRozH-rg). + +#### Prerequisites for CS API code generation +1. Get the source code of GTAD and its dependencies, e.g. using the command: + `git clone --recursive https://github.com/KitsuneRal/gtad.git` +2. Build GTAD: in the source code directory, do `cmake . && cmake --build .` + (you might need to pass `-DCMAKE_PREFIX_PATH=<path to Qt>`, + similar to libQuotient itself). +3. Get the Matrix CS API definitions that are included in the matrix-doc repo: + `git clone https://github.com/quotient-im/matrix-doc.git` + (quotient-im/matrix-doc is a fork that's known to produce working code; + you may want to use your own fork if you wish to alter something in the API). +4. If you plan to submit a PR or just would like the generated code to be + formatted, you should either ensure you have clang-format (version 6 at least) + in your PATH or pass the _absolute_ path to it by adding + `-DCLANG_FORMAT=<absolute path>` to the CMake invocation below. + +#### Generating CS API contents +1. Pass additional configuration to CMake when configuring libQuotient: + `-DMATRIX_DOC_PATH=<path you your matrix-doc repo> -DGTAD_PATH=<path to gtad binary (not the repo!)>`. + If everything's right, these two CMake variables will be mentioned in + CMake output and will trigger configuration of an additional build target, + see the next step. +2. Generate the code: `cmake --build <your build dir> --target update-api`; + if you use CMake with GNU Make, you can just do `make update-api` instead. + Building this target will create (overwriting without warning) `.h` and `.cpp` + files in `lib/csapi`, `lib/identity`, `lib/application-service` for all + YAML files it can find in `matrix-doc/api/client-server` and other files + in `matrix-doc/api` these depend on. +3. Re-run CMake so that the build system knows about new files, if there are any. + +#### Changing generated code +See the more detailed description of what GTAD is and how it works in the documentation on GTAD in its source repo. Only parts specific for libQuotient are described here. + +GTAD uses the following three kinds of sources: +1. OpenAPI files. Each file is treated as a separate source (because this is how GTAD works now). +2. A configuration file, in our case it's lib/csapi/gtad.yaml - this one is common for the whole API. +3. Source code template files: lib/csapi/{{base}}.*.mustache - also common. + +The mustache files have a templated (not in C++ sense) definition of a network +job, deriving from BaseJob; each job class is prepended, if necessary, with +data structure definitions used by this job. The look of those files is hideous +for a newcomer; and the only known highlighter that can handle the combination +of Mustache (originally a web templating language) and C++ is provided in CLion. +To slightly simplify things some more or less generic constructs are defined +in gtad.yaml (see its "mustache:" section). Adventurous souls that would like +to figure what's going on in these files should speak up in the Quotient room - +I (Kitsune) will be very glad to help you out. + +The types map in `gtad.yaml` is the central switchboard when it comes to matching OpenAPI types with C++ (and Qt) ones. It uses the following type attributes aside from pretty obvious "imports:": +* `avoidCopy` - this attribute defines whether a const ref should be used instead of a value. For basic types like int this is obviously unnecessary; but compound types like `QVector` should rather be taken by reference when possible. +* `moveOnly` - some types are not copyable at all and must be moved instead (an obvious example is anything "tainted" with a member of type `std::unique_ptr<>`). The template will use `T&&` instead of `T` or `const T&` to pass such types around. +* `useOmittable` - wrap types that have no value with "null" semantics (i.e. number types and custom-defined data structures) into a special `Omittable<>` template defined in `converters.h` - a substitute for `std::optional` from C++17 (we're still at C++14 yet). +* `omittedValue` - an alternative for `useOmittable`, just provide a value used for an omitted parameter. This is used for bool parameters which normally are considered false if omitted (or they have an explicit default value, passed in the "official" GTAD's `defaultValue` variable). +* `initializer` - this is a _partial_ (see GTAD and Mustache documentation for explanations but basically it's a variable that is a Mustache template itself) that specifies how exactly a default value should be passed to the parameter. E.g., the default value for a `QString` parameter is enclosed into `QStringLiteral`. + +Instead of relying on the event structure definition in the OpenAPI files, `gtad.yaml` uses pointers to libQuotient's event structures: `EventPtr`, `RoomEventPtr` and `StateEventPtr`. Respectively, arrays of events, when encountered in OpenAPI definitions, are converted to `Events`, `RoomEvents` and `StateEvents` containers. When there's no way to figure the type from the definition, an opaque `QJsonObject` is used, leaving the conversion to the library and/or client code. + ## How to check proposed changes before submitting them Checking the code on at least one configuration is essential; if you only have @@ -350,8 +371,7 @@ The following warnings configuration is applied with GCC and Clang when using CM `-W -Wall -Wextra -pedantic -Werror=return-type -Wno-unused-parameter -Wno-gnu-zero-variadic-macro-arguments` (the last one is to mute a warning triggered by Qt code for debug logging). We don't turn most of the warnings to errors but please treat them as such. -In Qt Creator, the following line can be used with the Clang code model -(before Qt Creator 4.7 you should explicitly enable the Clang code model plugin): +If you use Qt Creator, the following line can be used with the Clang code model: `-Weverything -Werror=return-type -Wno-c++98-compat -Wno-c++98-compat-pedantic -Wno-unused-macros -Wno-newline-eof -Wno-exit-time-destructors -Wno-global-constructors -Wno-gnu-zero-variadic-macro-arguments -Wno-documentation -Wno-missing-prototypes -Wno-shadow-field-in-constructor -Wno-padded -Wno-weak-vtables -Wno-unknown-attributes -Wno-comma`. ### Continuous Integration |