search

felias-fogg / SoftI2CMaster

Software I2C Arduino library
GNU General Public License v3.0
368 stars 100 forks source link

Moved guard to right position etc. #81

Closed ArminJo closed 11 months ago

ArminJo commented 11 months ago
ArminJo commented 11 months ago

Hello Bernhard, the error for ATTinyCore build is fixed by adding cli-version: 0.33.0 # to avoid errors for ATTinyCore I amended this change to my commit :-) Please merge this PR, because the wrong guard errror should be fixed at least. Thanks Armin from Cologne

felias-fogg commented 11 months ago

Hi Armin,

thanks for the PR. However, it is a lot at once!

First, there is the "cli-version ..." line. Can you tell me the background? I noted also at other points that there is a problem.

Second, you moved the guard, which is OK, I think.

Third, you introduced new functions. This means the version minor number should be bumped, and the functions should be explained in the interface section. Can you tell me the rationale behind it?

Fourth, there are formatting and rewriting changes, which I am OK with.

It would be preferable if you could submit PRs in smaller pieces so that I can review and then decide about them.

Since I am unsure about the additional functions, I will so far only add the guard movement and the cli-version line.

Thanks again for using and improving the library, Bernhard

ArminJo commented 11 months ago

Hi Bernhard, thanks for your fast feedback. Here is my answer.

  1. The reason are this issue https://github.com/arduino/arduino-cli/issues/2345 and this https://github.com/SpenceKonde/ReleaseScripts/issues/42#issuecomment-1739953510. Setting CLI version is the only workaround today 😞 .
  2. Sorry, I forgot to document the convenience functions. These are the functions I used so often in my programs, that I want to have them coded once and not beiing copied all over in my different sources. But at least I can put them in an extra file.
  3. Smaller PR's are a problem for me, but it is fine, if you just copy and paste the parts you are fine with and cancel this PR. Best regards Armin
ArminJo commented 11 months ago

Thanks for including the basic changes. Are you willing to include the convenience functuions and the additinal comments also? I can then prepare a new PR for this.

My opinion is, that convenience is a underestimated feature of libraries 😉

Best regards Armin