LiamBindle / MQTT-C

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

Issue/90 MQTT-C header rename #96

Closed vpetrigo closed 3 years ago

vpetrigo commented 4 years ago

Rename:

Update:

LiamBindle commented 4 years ago

@vpetrigo Thanks for this. Could you change mqtt_pal.c -> mqttc_pal.c and mqtt_pal.h -> mqttc_pal.h too?

vpetrigo commented 4 years ago

Should be done. 😄

yamt commented 4 years ago

what's the motivation of the rename?

yamt commented 4 years ago

answering myself, there's an issue with the explanation. https://github.com/LiamBindle/MQTT-C/issues/90

LiamBindle commented 4 years ago

@vpetrigo Just following up on this. I'll merge this when I have a chance to revise some of the documentation. It might be a little while, but this will get merged. Thanks for your patience.

vpetrigo commented 4 years ago

@LiamBindle just FYI there were some merge conflicts that I just resolved.

vpetrigo commented 3 years ago

Hey @LiamBindle, would you tell if you have timeline in mind when this PR will be merged? 😄 I just realised that there another bunch of conflicts and I'm wondering whether I should them resolve now.

LiamBindle commented 3 years ago

@vpetrigo I apologise for the slow response.

The issue I'm running into is these changes break compatability and therefore warrant a major version upgrade (i.e., if someone does git pull it will break their project, t.f., this would need to go into a version 2.0). But I don't think renaming files is a substantial enough upgrade to warrant a major version release.

I haven't heard other requests for this, and it could alternatively be solved by contraining your include directories, so I'm hesitant to merge this. What do you think?

Perhaps, @learn-more @jepaan @yamt @markrad, do you have any thoughts on whether mqtt.c should be renamed to mqttc.c?

vpetrigo commented 3 years ago

@LiamBindle, I may suggest bundling a mqtt.h that will include renamed mqttc.h. That file may be deleted later in v2.0+ for example when people most likely move to a mqttc.h header file.

learn-more commented 3 years ago

I have no strong preference about the naming, but if you want to prevent naming conflicts a header structure like <mqttc/mqttc.h> seems even more distinct.

the mqtt.h in-between header seems like a nice touch, you could generate a warning in that before including the real header, but that does not solve the conflicting header issue right now?

vpetrigo commented 3 years ago

If we put renamed mqttc.h into a mqttc subfolder in the include/ directory – it will solve.

yamt commented 3 years ago

the compat mqtt.h would make my life as a consumer of this library a bit easier.

LiamBindle commented 3 years ago

The mqtt.h compatability file would cause the same error this PR is intended to fix though.

This issue only affects a small portion of users, and the fix would break compability for a much larger portion of users. Additionally, for any users that are affected by conflicting header file names, they could simply restrict their include directories or rename the file.

Since there isn't substantial support for this and it would break compatibility for a larger user group than it would help, I'm going to close this PR. I hope you understand. I'm happy to discuss further if you would like. Hope to see more PRs in the future!

it is relatively easily fixable, and the fix would break compatibility, I'm going to close this PR.

vpetrigo commented 3 years ago

So, do you mind me making the following changes:

include "mqttc/mqttc.h"

warning "This file might be deprecated in the upcoming versions"

endif / __MQTT_GUARD __ /

// mqtt.h

ifndef MQTT_PAL_GUARD

define MQTT_PAL_GUARD

include "mqttc/mqttc_pal.h"

warning "This file might be deprecated in the upcoming versions"

endif / MQTT_PAL_GUARD /



?

That help me as a user of that library to specify `include/mqttc` as a root include directory for the project, so that it resolves the issue for my case and will not break existing code.
LiamBindle commented 3 years ago

I'd suggest doing this your fork. Alternatively you could create a new directory mqttc/ and symlink ../src and ../include and that would fix your problem too.

MQTT-C is stable, so this would be easy to maintain moving forward. In terms of the real file structure, I would like to keep it as is.

vpetrigo commented 3 years ago

Got it, Liam. Thank you for your patience. 😄

LiamBindle commented 3 years ago

No problem! Thanks for your contributions to MQTT-C :smile: I hope you'll continue to submit PRs!

markrad commented 3 years ago

Late to the party but I am inclined to agree with Liam on this. I looked at the lwip referenced in the original issue and, from my perspective, the issue should be posted on their repo not this one. They have crammed the headers for multiple protocols into a single directory meaning if you want to use their http for example you get stuck with mqtt.h in your search path too. I think it is perfectly reasonable for an app that is solely an MQTT client to have a header called mqtt.h. This only is a problem because of the armmbed header layout.