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

Added multi testing for digital pins (via array) #3

Merged
merged 4 commits into from
May 13, 2022

Conversation

DreiDe
Copy link
Member

@DreiDe DreiDe commented May 13, 2022

This Pull Request implements validation of multiple pins as requested in #1 . Thus making it simpler to validate more than one pin.

In addition to bool isDigitalPinValid(uint8_t pin); a function bool isDigitalPinsValid(uint8_t pins[], uint16_t pinsLength); is now available.

@DreiDe DreiDe linked an issue May 13, 2022 that may be closed by this pull request
@DreiDe DreiDe self-assigned this May 13, 2022
@DreiDe DreiDe added the enhancement New feature or request label May 13, 2022
@DreiDe DreiDe changed the title Added multiesting for digital pins (via array) Added multi testing for digital pins (via array) May 13, 2022
@DreiDe
Copy link
Member Author

DreiDe commented May 13, 2022

@happybrick

I had a couple of complains:

  • The function name isDigitalPinValid is hard to distinguish from isDigitalPinsValid according to coding standards:
    image
  • The new function should make things easier so why not use it everywhere in the library internally?
  • files did not have newlines applied to them
  • code formatting has not been run

What i've done:

  • Rename functions to isDigitalPin and isAnalogPin
    • breaking change
    • does not fix any of the errors above but is nicer to read (in my opinion) and equally simple to understand
  • implement multi validator as function overload to isDigitalPin
  • run code formatting
  • add newlines

While reviewing I noticed three other things:

  • it would be appreciated if the same overload (multi pin validation) would also exist for isAnalogPin Multi Pin validation for isAnalogPin #5
  • it would be nice if the coding conventions were structured in chapters like 1.1, 2.6 e.g. so they can easily be referenced inside a code review https://github.com/QiTech-Industries/Wiki/issues/2
  • versioning is a good idea so i introduced the current version as v0.0.0 and as the two currently open Pull Requests will get merged, we have v0.1.0

@DreiDe
Copy link
Member Author

DreiDe commented May 13, 2022

@happybrick

  1. Hast du den Code auf deine breaking change umbenannte Methode angepasst?

Yes, at least in the library itself. Im going to adjust the winder version as soon as the change is in main.

  1. Du müsstest mir dann am Montag zeigen was ich genau machen muss um das Code Formatting rüberlaufen zu lassen damit das passt

Just press Ctrl+Alt+F (also documented in the Editor Settings section in the coding conventions)

  1. Mein Code war als Vorschlag der Umsetzung und nicht schon als kompletter Pullrequest vorgesehen

At some point we have to merge changes in the main branch so just open a pull request and assign me as a reviewer once you finished working on a issue or implemented a new feature.

  1. Wäre ggf. die Nutzung einer Map-Funktion sauberer? (nur als idee)

I don't think the map function provides any advantages in terms of code readability or code length here. So I would stick with a simple for loop.

  1. Die Nummerierung der Coding Conventions ist praktisch, jedoch darf man dann nie etwas entfernen sondern immer nur streichen um die Referenzen gültig zu halten

That's true. But I don't believe that there is an option to get a permalink to a single chapter anyway. And once a pull request or issue is closed the references don't really matte either. You can add your concerns in a comment on https://github.com/QiTech-Industries/Wiki/issues/2

@DreiDe DreiDe requested a review from happybrick May 13, 2022 08:00
@DreiDe
Copy link
Member Author

DreiDe commented May 13, 2022

@happybrick
If you accept my changes just approve the PR so I can merge it. I added you as a reviewer. If you have any suggestion changes just comment in your review.

@DreiDe DreiDe merged commit 1cc34cf into main May 13, 2022
@DreiDe DreiDe deleted the 1-make-isdigitalpinvalid-accept-pin-array branch May 13, 2022 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make isDigitalPinValid() accept pin array
2 participants