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

Fix compile warnings breaking builds with -Werror. #31

Closed
wants to merge 4 commits into from

Conversation

LiSongMWO
Copy link

Ran into some problems pulling in hueplusplus into my project that uses strict flags for warnings.

Fixes some sign mismatches and implicit fall through and missing initializer.

@codecov
Copy link

codecov bot commented Mar 19, 2020

Codecov Report

Merging #31 into master will increase coverage by 0.04%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
+ Coverage   88.61%   88.66%   +0.04%     
==========================================
  Files          37       37              
  Lines        3592     3590       -2     
==========================================
  Hits         3183     3183              
+ Misses        409      407       -2     
Impacted Files Coverage Δ
hueplusplus/LinHttpHandler.cpp 0.00% <0.00%> (ø)
hueplusplus/include/Hue.h 100.00% <ø> (ø)
hueplusplus/HueCommandAPI.cpp 98.24% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a3b8d6...36a58ad. Read the comment docs.

@enwi
Copy link
Owner

enwi commented Mar 19, 2020

Hi @EmilyBjoerk and thanks for the work.

Have you checked if a modification of the included json library is really necessary?

The version we are using here is pretty old now and I saw a PR on that topic -> will this fix the issue as well? Then we can just update the library

@LiSongMWO
Copy link
Author

Hi @EmilyBjoerk and thanks for the work.

Have you checked if a modification of the included json library is really necessary?

The version we are using here is pretty old now and I saw a PR on that topic -> will this fix the issue as well? Then we can just update the library

I think pulling a newer version from upstream might fix it, we can try that first.

Co-Authored-By: Moritz Wirger <lj.moritorii@web.de>
@LiSongMWO
Copy link
Author

Please let me know when you have updated the json library from upstream, their file structure seemed significantly different from what I saw in your repo. I'm not sure if they compile the source into one file or if your version is prior some refactoring so I'm not confident doing the update in this PR.

If the user wishes to connect to a specific bridge it makes sense for them to remeber the bridge by and scan for the bridge by comparing MAC addresses as these are more stable than IPs in an ipv4 DHCP network. In order for the MAC address comparison to be stable, the user should use the same normalisation scheme as the library. Exposing this function to the user makes their life easier.
@enwi
Copy link
Owner

enwi commented Mar 19, 2020

I switched to another JSON library nlohmann-json, which I wanted to do for some time now, the changes are on the development branch

@LiSongMWO
Copy link
Author

I'm unfamiliar with your git workflow, looking at the history I would have expected to see back merges from master to development but it seems you rebase/cherrypick instead? I'm not quite sure how you want me to do this.

The changes I've made are basically trivial and I wonder if it isn't easier for you to just replicate them with your familiar workflows at this point? I've forked and fixed what I needed on my fork and now completed/deployed my project already so I'm not that concerned one way or another about the contribution "credit".

@enwi
Copy link
Owner

enwi commented Mar 19, 2020

@EmilyBjoerk No worries, changes are pushed and pullrequests are merged with the development branch first, then once the changes have been checked and they're stable they get merged into the master branch technically creating a new version of the library. Maybe it's time to create a contribution guide.

Also nice project BTW 😄

enwi added a commit that referenced this pull request Mar 20, 2020
@enwi
Copy link
Owner

enwi commented Mar 20, 2020

@EmilyBjoerk I've tested your code with the changes made on the development branch and it all works fine.

@enwi enwi closed this Mar 20, 2020
Jojo-1000 pushed a commit that referenced this pull request Mar 31, 2020
@enwi enwi mentioned this pull request Mar 31, 2020
enwi added a commit that referenced this pull request Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants