search

LiamBindle / MQTT-C

A portable MQTT C client for embedded systems and PCs alike.
https://liambindle.ca/MQTT-C
MIT License
774 stars 275 forks source link

Split mqtt.h in detail and api headers #52

Closed learn-more closed 4 years ago

learn-more commented 5 years ago

For an end-user none of the detail api are interesting, so by moving them to a new header (mqtt_internal.h for example) this is less visible for the end user.

This leaves a cleaner user-facing header, and exposes less implementation details.

LiamBindle commented 5 years ago

@learn-more I agree that moving the details declarations to their own header would be cleaner, but the reason I originally decided against this was that I really wanted MQTT-C to be one header and one source file. However, for end-users, this made implementing the PAL more daunting that it actually is, which is why I pulled the PAL out into it's own files.

I get the impression end-users' projects are pretty evenly split between embedded systems and PCs. I really want to make sure I keep the embedded-systems people happy though, because that's where I see MQTT-C's niche. Part of that IMO (let me know if you feel otherwise), is making it "easy" to copy-paste MQTT-C's files into other projects. By "easy" I mean I want it to be obvious that copy-pasting MQTT-C into your project is going to be easy. And IMO, part of that is minimizing the number of files in MQTT-C and keeping the directory structure as simple as possible.

That's actually also why I didn't add a proper build system originally, but I think a simple set of CMakeLists doesn't make MQTT-C appear more complex than it actually is.

Interested to hear what you think though!

learn-more commented 5 years ago

The first thing I do when using a library is opening the header, seeing where to start. It starts with a license, then some explanation about the examples, followed by something from the group unpackers, with some error-related stuff from the api group. Then, ~1000 lines of internal stuff, followed by a bunch of api definitions starting at line ~1200, interleaved with internal functions.

In this case, my opinion would be that splitting this file into 2 would be worth it, but ultimately, you are the author of this library, and if you would add a new file every time someone asks for it you'll end up with a huge repo at the end of the line.

LiamBindle commented 5 years ago

Yeah, that's fair. I don't see a reason all the internal details' delcarations couldn't be made static declarations in mqtt.c though. Maybe that would be a good way to hide the details without adding a new file.

What do you think?

learn-more commented 5 years ago

As long as they are not needed for pal this seems like a reasonable compromise. I would suggest to maintain some kind of structure, trying to group related stuff together.

learn-more commented 5 years ago

Another option would be to split it out in multiple files, and for source distributions use some script to combine them in one header and one .c file. (googletest used to do this, not sure if they still do that)

LiamBindle commented 5 years ago

As long as they are not needed for pal this seems like a reasonable compromise.

Yeah, this should work.

I would suggest to maintain some kind of structure, trying to group related stuff together.

Good idea