search

claws / BH1750

An Arduino library for the digital light sensor breakout boards containing the BH1750FVI IC
MIT License
248 stars 107 forks source link

Modified library initialization to be compatible with modern libraries: #50

Closed brezuicabogdan closed 4 years ago

brezuicabogdan commented 5 years ago
claws commented 5 years ago

A good merge request should ideally contain just the change being proposed. There is a lot of unnecessary formatting changes that obscure the actual change you are proposing.

  1. Can you update the merge request to only contain the change(s).
  2. The simple Travis CI step is failing with your proposed changes because the examples have not been updated to use your proposed changes.
  3. Multiple people have already contributed without adding their names into the source code. The repo history clearly shows who's contributed so I don't think there is a need to add your name to the code comments.
  4. Can you provide a pointer to information regarding the "modern" libraries you refer to? Is there some style guide you are trying to be compliant with?
moritz89 commented 4 years ago

Hi, is there still interest to merge the pull request? I would be interested in being able to pass a TwoWire reference/pointer as at least on the ESP32, there are two I2C interfaces.

claws commented 4 years ago

Conceptually, yes. This actual MR has too many issues in its current state. I'd recommend proposing your change in a new merge request. Try to describe the problem you are solving - from your comment it seems like a valid problem to solve. Similar to this MR I'd recommend the proposed solution use some form of optional param so as to maintain the existing API.