Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QEP 400: Code style and practice guidelines #314

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

nyalldawson
Copy link
Contributor

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
Copy link
Contributor Author

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

qep-400-coding-practice.md Outdated Show resolved Hide resolved
nyalldawson and others added 2 commits December 31, 2024 21:17
Co-authored-by: Harrissou Sant-anna <[email protected]>
@uclaros
Copy link

uclaros commented Dec 31, 2024

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

Do we actually need to have a QEP numbering system that does not match the PR/issue number? It seems to me like a potential point of confusion and a burden to keep track of!
314 (being 100 * π) is an easy to remember number for the first PR based QEP :)

@nyalldawson
Copy link
Contributor Author

@uclaros

Do we actually need to have a QEP numbering system that does not match the PR/issue number?

I can certainly change it, but keep in mind that we'll end up with big gaps in the numbers that way, since issues and pr numbering is shared but only prs will be QEPs. It's also possible now to have multiple prs target the same QEP (eg a future change to these specifications is modifying the same QEP from a different pr number). Given this, I don't see any benefit in forcing the numbering to match the first pr creating a new QEP.

@uclaros
Copy link

uclaros commented Jan 1, 2025

I can certainly change it, but keep in mind that we'll end up with big gaps in the numbers that way, since issues and pr numbering is shared but only prs will be QEPs. It's also possible now to have multiple prs target the same QEP (eg a future change to these specifications is modifying the same QEP from a different pr number).

Do we mind if there are gaps? We were using the issue number until now so gaps were inevitable.
Regarding multiple PRs for a single QEP we can have a QEP label applied to the main PR and keep referencing it by its number.

I'd rather have gaps in the numbering than having to search the closed PRs list for the last QEP number to increment, unless there's some simple way to automate this.

@elpaso
Copy link

elpaso commented Jan 1, 2025

@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?

Copy link

@troopa81 troopa81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@uclaros
Copy link

uclaros commented Jan 2, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants