search

atomicobject / heatshrink

data compression library for embedded/real-time systems
ISC License
1.31k stars 176 forks source link

Add option to override heatshrink configuration #63

Closed ajaybhargav closed 3 years ago

ajaybhargav commented 3 years ago

Currently its impossible to change heatshrink compression configuration wihout touching the library header files. This patch makes it possible for configuration which can be configured at compilation time without having to touch the library. Which makes it easier to keep it sync with github source without modifying the actual source.

Signed-off-by: Ajay Bhargav contact@rickeyworld.info

silentbicycle commented 3 years ago

I don't know who the hell @BenBE is, but they are not in any way affiliated with this project, and their review approval means nothing.

silentbicycle commented 3 years ago

The changes look good to me, but as noted in CONTRIBUTING.md, the PR needs to be targeted to the develop branch. I will retarget it when I prepare the next versioned release (which should happen in the next couple weeks), unless you get to it first.

BenBE commented 3 years ago

I don't know who the hell @BenBE is, but they are not in any way affiliated with this project.

Looking at my profile, you might notice several FLOSS projects like htop (cross-plattform system monitor) and GeSHi (syntax highlighter) where I'm in the core developer team / maintainer. Apart from this I'm using this library here and thus am following its development. AFAIR it's not prohibited, to acknowledge that a PR and its patches look fine. ;-) Anybody can do this, and to the contrary: with bad code everybody even should do voice their issues with PRs they come across.

Apart from this, just looking at the commit history in this project, one might get the impression that it has been abandoned by its original developers; and wasn't it for several other projects I already help maintain I'd have taken up this task already long ago. Just looking at the network graph of commits that could/should be backported into this main repository is kinda sad.

silentbicycle commented 3 years ago

AFAIR it's not prohibited, to acknowledge that a PR and its patches look fine. ;-)

It doesn't, the CONTRIBUTING.md says:

Please send patches or pull requests against the develop branch.

Consider that you are actually making things worse, and your "help" is unwanted.

The project is not abandoned, I am planning on doing another versioned release soon (after another contribution happening via channels outside github is done), but I am very busy with other things and it only gets a limited amount of attention.

ajaybhargav commented 3 years ago

The changes look good to me, but as noted in CONTRIBUTING.md, the PR needs to be targeted to the develop branch. I will retarget it when I prepare the next versioned release (which should happen in the next couple weeks), unless you get to it first.

Sorry I missed that. If needed I can generate pull request on develop branch instead.

silentbicycle commented 3 years ago

That may be for the best. Normally I can edit which branch the PR targets (and it looks like this one would port over cleanly), but in this case I don't seem to be able to, possibly because it's an organization repo rather than a personal one? You probably can, and if not, ordering a second PR would be fine.

Targeting develop is so that everything can be merged as part of versioned releases.

As long as it targets develop, the code change is good. I add that kind of #ifndef wrapper by habit now, and I thought heatshrink already did it. In retrospect it is definitely missing.

ajaybhargav commented 3 years ago

somehow changing base shows conflict. So I created pull request #64