Skip to content

Conversation

arturo182
Copy link
Collaborator

No description provided.

@arturo182 arturo182 requested a review from tannewt July 9, 2018 04:44
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Way cleaner. Thank you!

@tannewt tannewt merged commit e875f4e into adafruit:master Jul 10, 2018
@arturo182 arturo182 deleted the nrfx_gpio branch July 10, 2018 05:45
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are followup comments to an already approved PR.

digitalio_digitalinout_obj_t* self) {
if (self->open_drain) {
digitalio_digitalinout_obj_t *self) {
if (self->open_drain)
return DRIVE_MODE_OPEN_DRAIN;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, etc.

// True is implemented differently between modes so reset the value to make
// sure its correct for the new mode.
if (value) {
if (value)
Copy link
Collaborator

@dhalbert dhalbert Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is already approved, but I would ask you not to remove braces around single statements in control structures. Please always use braces in control structures, except perhaps in the most extreme cases (like very stylized single-line code). I've had to fix several bugs over the years that were caused by edits to code with missing braces.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'll try to remember, at this point it's muscle memory 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants