Bringing the tree up to formatting parity with clang-format, shellcheck, shfmt, yaml linting, and MD lint #432
Conversation
woody-apple
requested review from
anush-apple,
BroderickCarlin,
chrisdecenzo,
gerickson,
hawk248,
jelderton,
robszewczyk and
tpmanley
as code owners
| #include "LEDWidget.h" | ||
| #include "AppTask.h" |
There was a problem hiding this comment.
wonder what rule this is for
There was a problem hiding this comment.
Probably adding your own header include at the top. Seems reasonable to me. It's in the clang format :)
There was a problem hiding this comment.
Might only be applying this to C++, looking...
There was a problem hiding this comment.
Actually, no clue. I'm not a clang-format expert. I checked the sorting options available, and not clear to me the logic it's applying. It's stable now, so unless you know which option you'd like me to check, I'd love for that to be a different PR (happy to re-format then too)
There was a problem hiding this comment.
Scratching that, not a clang-format expert. Happy to change the format as needed. I'm just mechanically applying the existing format to everything.
There was a problem hiding this comment.
this one looks correct to me, just baffled that it applied another rule to init_lcd
| @@ -177,7 +176,7 @@ inline void BLEManagerImpl::SetAdvertisingHandle(uint8_t handle) | |||
|
|
|||
| inline BleLayer * BLEManagerImpl::_GetBleLayer() const | |||
| { | |||
| return (BleLayer *)(this); | |||
| return (BleLayer *) (this); | |||
| @@ -606,7 +707,10 @@ inline void Put16(uint8_t *p, uint16_t v) { nl::IO::BigEndian::PutUnaligne | |||
| * swapped. | |||
| * | |||
| */ | |||
| inline void Put32(uint8_t *p, uint32_t v) { nl::IO::BigEndian::PutUnaligned32(p, v); } | |||
| inline void Put32(uint8_t * p, uint32_t v) | |||
There was a problem hiding this comment.
these changes make me worry that restyled might be using a different configuration than we'd already settled on (clang-format-9 with the current ToT/.clang-format).
the reason it makes me worry is because I believe these files were reformatted in a PR of mine to that configuration...
There was a problem hiding this comment.
nevermind. I think this must have been a file outside of "make pretty" at the time. My editor is happy with this formatting.
There was a problem hiding this comment.
I agree -- the existing make pretty-check that is run on every PR now should be enforcing compliance on all files under src, so any style changes on those files should be viewed with suspicion.
Perhaps this shows that headers are escaping the current checks?
There was a problem hiding this comment.
Can confirm it only uses the clang format supplied. Ran same command by command line. This file was outside make pretty
There was a problem hiding this comment.
Note, make-pretty doesn't hit all files. Coming up with a solution later that replaces it with something that will do it without developer configuration.
There was a problem hiding this comment.
I agree -- the existing
make pretty-checkthat is run on every PR now should be enforcing compliance on all files undersrc, so any style changes on those files should be viewed with suspicion.Perhaps this shows that headers are escaping the current checks?
@turon we're not doing make pretty-check on PRs anymore
There was a problem hiding this comment.
FYI: You're welcome to, but it's an un-needed task now, and doesn't have the same breadth as restyled. I'm working a local restyled for those that want to run it before the PR.
…D4166A Merge in WMN_TOOLS/matter from thermostat_fix to silabs_slc_1.0 Squashed commit of the following: commit 267b7c99fd9565346fa35c35d99d5b0da06b8454 Author: Hussein Elsherbini <[email protected]> Date: Tue Dec 13 22:34:16 2022 -0500 adds an exclusion of thermostatUI.h thermostatIcons.h and thermostatUI.cpp if brd4166a is target
Problem
make-pretty doesn't check much of the source tree, let's not push the burden of people fixing restyled ad hoc, pull the bandaid!
Summary of Changes
Restyle all the things from a command line too. Changes to come separately for that.