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

Moved guard to right position etc. #81

Closed
wants to merge 1 commit into from

Conversation

ArminJo
Copy link

@ArminJo ArminJo commented Oct 20, 2023

  • Moved guard to right position
  • Added comments
  • Added convenience functions
  • Changed "#ifndef" to "#if !defined()"
  • Updated Copyright and version number
  • Fixed typo

…tions and changed "#ifndef" to "#if !defined()"
@ArminJo
Copy link
Author

ArminJo commented Oct 20, 2023

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
Copy link
Owner

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
Copy link
Author

ArminJo commented Oct 20, 2023

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

  1. The reason are this issue Package index filename is truncated if it contains . arduino/arduino-cli#2345 and this http://drazzy.com/package_drazzy.com_index.json is not working anymore for Github actions SpenceKonde/ReleaseScripts#42 (comment). 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
Copy link
Author

ArminJo commented Nov 3, 2023

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

@ArminJo ArminJo closed this Nov 3, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants