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

Move headers to dedicated libquotient directory #445

Closed
AlexandrePTJ opened this issue Jan 27, 2021 · 7 comments · Fixed by #626 · May be fixed by #462
Closed

Move headers to dedicated libquotient directory #445

AlexandrePTJ opened this issue Jan 27, 2021 · 7 comments · Fixed by #626 · May be fixed by #462
Labels
enhancement A feature or change request for the library

Comments

@AlexandrePTJ
Copy link

AlexandrePTJ commented Jan 27, 2021

Description

I am starting a new application based on libQuotient but I am a bit disappointed with headers organisation.

From what I see, I have to include libQuotient headers like this:

#include <connection.h>
#include <settings.h>

I feel that, as this are relatively common file names, it will conflict with end users project headers.
It should be more convenient and standard for a library to expose headers like this:

#include <libquotient/connection.h>
// or
#include <quotient/settings.h>
@CarlSchwan
Copy link
Contributor

I agree that the headers have some high change to conflict with the headers from the application. This is also why we have neochatroom.h and neochatuser.h in NeoChat.

@KitsuneRal KitsuneRal added the enhancement A feature or change request for the library label Jan 28, 2021
@KitsuneRal
Copy link
Member

KitsuneRal commented Jan 28, 2021

TL;DR: yes, certainly; I'm just unsure about capitalisation. The current widespread packaging uses Quotient/; but quotient/ might be better. And while we're at it, maybe make file names snake/camel-cased (see the bottom)?

The below is for the sake of documenting the considerations.
For the sake of pedantry - you can use <settings.h> vs. "settings.h" to discern between "system-wide" (library) and "user" (relative) header files. With that said, if there are settings.h headers in two dependent libraries and you happen to need them in the same compilation unit, you're toast.

I thought I'd leave the directory for distros to define (e.g. Debian would use libquotient/ while Fedora would stick with Quotient/) - but I was ignorant: Debian-based distros actually package files to libquotient/Quotient while virtually every other major breed simply uses Quotient/, conventionally defined in my CMakeLists.txt. And even C++ Core Guidelines recommend that. So, yeah, sure.

Meanwhile, there's a bit of misalignment between capitalised Quotient and all-lowercase header file names. Besides, even I, myself struggle to read words glued together in the longer file names like roomcanonicalaliasevent.h. Options I think of:

  • Current style: <Quotient/settings.h>, <Quotient/events/roommessageevent.h>
  • Qt style: <Quotient/Settings>, <Quotient/Events/RoomMessageEvent> (I personally am predisposed against having extensionless file names but it seems to also be used in KDE)
  • Clang-style: <Quotient/Settings.h>, <Quotient/Events/RoomMessageEvent.h>
  • Style derived from C++ Core Guidelines defaults: <quotient/events/room_message_event.h>

Thoughts?

@AlexandrePTJ
Copy link
Author

From my side, I prefer snake over camel, but then it is up to you.

Thanks you for the "pedantry" explanation :)

@jarjarfan666
Copy link

I like Qt style, and vote for Clang as my second choice.

@CarlSchwan
Copy link
Contributor

I like the Qt style the most and it would make sense since it's a Qt library. This is also what KDE Frameworks are doing. You might want to take a look at https://invent.kde.org/frameworks/extra-cmake-modules/-/blob/master/modules/ECMGenerateHeaders.cmake for the generation of the headers in this style.

@KitsuneRal
Copy link
Member

Oh, you even have an ECM macro for that... It doesn't like snake_case headers though, unless I missed something. That basically limits options for (non-forwarding) headers to Clang-style and current style. And yeah, I'll have to consider using Qt-style forwarding headers.

@KitsuneRal
Copy link
Member

The original concern is addressed by #626; renaming of header files is a subject of #462.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or change request for the library
Projects
None yet
4 participants