-
Notifications
You must be signed in to change notification settings - Fork 523
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
Add braces around blocks. #999
Conversation
Codecov ReportBase: 50.94% // Head: 50.94% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #999 +/- ##
==========================================
- Coverage 50.94% 50.94% -0.00%
==========================================
Files 378 378
Lines 31658 31659 +1
==========================================
Hits 16126 16126
- Misses 15532 15533 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
This pull request is in conflict. Could you fix it @corycrean? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great readibility improvement! I think in theory there is nothing that could really go wrong, but this is really a lot to review
4525219
to
d8f0c91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
This pull request is in conflict. Could you fix it @corycrean? |
1 similar comment
This pull request is in conflict. Could you fix it @corycrean? |
Wow, this is a huge change. Thank you for doing this work. I'm happy to merge this if you don't mind rebasing so it can be applied. |
@corycrean I'm sorry this got so out of date and has so many conflicts. Do you think you could rebase this and make it so it could be merged? |
d8f0c91
to
ef431d7
Compare
// Return first feasible solution | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you run pre-commit does the formatting of this get fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately no. But I just fixed this manually.
@@ -11,6 +11,7 @@ Checks: '-*, | |||
modernize-make-unique, | |||
modernize-avoid-bind, | |||
misc-unused-parameters, | |||
readability-braces-around-statements, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a formatting issue got through in this PR
I wanted to point out how the argument that drove the creation of #854 only applies to codebases without autoformatters. Misleading indentation is a real problem but it's not a problem MoveIt has so there was no need to add this check to address that. We already have no misleading indentation. |
Description
This PR addresses #854.
Checklist