QEP 314: Code style and practice guidelines

[nyalldawson]

This QEP documents coding standards and conventions used throughout the QGIS code base. Developers are required to follow these standards when submitting code to QGIS.

It is initially intended as a formal description of CURRENT CODE PRACTICES ONLY, which can be later modified and revised (in an atomic manner) if we want to change any of these standards. As such, please refrain from using this initial request as a place to discuss changes to current practice. Rather, let's keep discussion focused on whether this is an accurate and complete description of our current coding practices. 👍

The end goal here is having a formal policy in place describing all our coding conventions, which will help ease new developers into contributing effectively to QGIS and remove some of the unspoken assumptions surrounding QGIS development.

[nyalldawson]

Note: I've arbitrarily restarted the number at 400 here -- this seems right after the restructuring of QEPs to PRs.

[elpaso]

@nyalldawson looks goo to me, thank you for taking care of this.

Should we also mention the use of (possibly redundant but explicit) std::as_const in for loops?

[troopa81]

Should we also include something about using the new settings API instead of the raw string Qt api?

[uclaros]

Some more coding conventions we use that are not listed:

• Header/license for each file
• Includes
  • Separate qgis from qt includes
  • Include moc file for qobjects
  • Include qt as #include <QMap> and not #include "qmap.h"
  • Prefer forward declaration if possible instead of including from within header files
• Use QStringLiteral for untranslated literals, QLatin1String for comparisons
• Don't use qDebug(), use QgsDebugError/QgsDebugMsgLevel (we still need to define some logic for which level < 1 to use)
• methods that are actually const should be marked const

[troopa81]

@uclaros Maybe that should come in a separate PR. Do you mind if I merge this one and we iterate later on it?

[nyalldawson]

@troopa81

Please don't merge -- I want this initial version to be complete.

[troopa81]

Please don't merge -- I want this initial version to be complete.

OK

[m-kuhn]

@nyalldawson I have converted to draft, with the approvals in place already it will be easier to monitor, when you are happy with it.

[3nids]

One small question regarding the process and scope of QEPs.

To me a QEP should be definitive once merged/accepted.

For this one, it looks like it will be a continuous evolution. So is this the correct place?

For code style, we already have this: https://github.com/qgis/QGIS-Documentation/blob/master/docs/developers_guide/codingstandards.rst which sounds more appropriate than a QEP.

[nyalldawson]

@uclaros
Renamed to 314.

[nyalldawson]

@elpaso

Should we also mention the use of (possibly redundant but explicit) std::as_const in for loops?

Ok, I've added that for explicitly non const containers only. I don't think we currently have agreement on whether this should be required for already const containers, so if you want that then let's start that change discussion separately after this QEP is formalised.

[nyalldawson]

@troopa81

Should we also include something about using the new settings API instead of the raw string Qt api?

No, IMO that one would belong in a formal policy regarding settings usage. (Ideally with code examples for both C++ and python)

[nyalldawson]

@3nids

One small question regarding the process and scope of QEPs.
To me a QEP should be definitive once merged/accepted.
For this one, it looks like it will be a continuous evolution. So is this the correct place?

Yes, that's correct. Since #310 QEPs can now be mutable, exactly so that we have a way of formally handling mutable QEPs just like this one.

There's a mix of needs for QEPs. We have short lived project based ones like #318 which likely will remaining unchanged forever after the initial acceptance. Then we have "living" specs like this one, or eg one for backport policies, pull request standards, API stability requirements, etc. These ones are inherently mutable and trying to handle them via seperate independent QEPs where every change is a new document is impractical. The new approach allows for a single point of truth for living specifications like this.

For code style, we already have this: https://github.com/qgis/QGIS-Documentation/blob/master/docs/developers_guide/codingstandards.rst which sounds more appropriate than a QEP.

That page should be removed and changed to a link to this formal specification after merging.

[nyalldawson]

@uclaros

Separate qgis from qt includes
Include qt as #include and not #include "qmap.h"
Use QStringLiteral for untranslated literals, QLatin1String for comparisons
Don't use qDebug(), use QgsDebugError/QgsDebugMsgLevel

Thanks, I've added these

Include moc file for qobjects
Header/license for each file

These are already explicitly covered by tests on the CI, I'll omit them as a result.

Prefer forward declaration if possible instead of including from within header files
methods that are actually const should be marked const

These fall under "best practice modern C++" , so I don't want to explicitly add them here. (And the second is already covered by cppcheck on CI)

[nyalldawson]

Ok, all comments addressed. How are we looking now?

[elpaso]

@elpaso

Should we also mention the use of (possibly redundant but explicit) std::as_const in for loops?

Ok, I've added that for explicitly non const containers only. I don't think we currently have agreement on whether this should be required for already const containers, so if you want that then let's start that change discussion separately after this QEP is formalised.

IIRC it was @m-kuhn back in the days who corrected my code when I didn't use as_const over an already const container. I have always thought that it was pointless but also harmless (it would be a no-op in that case), I'm fine with not requiring as_const over an already const container.

[m-kuhn]

I do think that reviewer time and focus is not best invested to check if a variable over which we loop is already const in a given context, but I am open how we handle this. I would keep this discussion separate and not block this, given there is no agreement yet.