Bluetooth: Coding style documentation

zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.

https://docs.zephyrproject.org

Apache License 2.0

10.61k stars 6.5k forks source link

Bluetooth: Coding style documentation #55281

Open jori-nordic opened 1 year ago

jori-nordic commented 1 year ago

Reviewers for Bluetooth patches seem to have a clear idea of the coding standard that the Bluetooth subsystem is supposed to follow, but this isn't documented anywhere.

Create a "coding style" page with all the do's and don'ts here https://docs.zephyrproject.org/latest/connectivity/bluetooth/index.html

It can include stuff like doxygen commenting style, newline before return, scope of vars, etc..

Thalley commented 1 year ago

We used to have a discussion that was closed due to inactivity too: https://github.com/zephyrproject-rtos/zephyr/discussions/41120

There are a lot of code styles that we haven't agreed on, and thus probably shouldn't be enforced.

jori-nordic commented 1 year ago

I think we can state somewhere that they aren't "rules" per se, but rather recommendations and mention that it's a rather informal and living document. Then you can make a PR, doesn't have to be an exhaustive list, just the ones you feel the most strongly about. Does that sound good?

edit: the purpose of this task really is that once we have that document, a style comment on a PR should have a corresponding entry in that document, or shouldn't be made. That way we cut down on the review noise and can focus on the actual behavior/functionality.

Thalley commented 1 year ago

the purpose of this task really is that once we have that document, a style comment on a PR should have a corresponding entry in that document, or shouldn't be made.

That assumes that we can agree on such rules :D And we would also need to ensure that they do not conflict with any of the non-BT Coding Guidelines (https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html)

That way we cut down on the review noise and can focus on the actual behavior/functionality.

IMO the only way we can really achieve this, is if we can have a tool in CI run these tests, but that would require general support from Zephyr, as such a tool should apply for the entire Zephyr project.

Personally I agree with your statements, but having tried to walk down the path of BT specific guidelines, I don't have high hopes for a solution. I won't take the lead on this, but I'll support anyone that will :)

jori-nordic commented 1 year ago

Alright, I'll assign it to myself then. I'll try to start with a small subset we can all agree on.

jori-nordic commented 1 year ago

another instance of this https://github.com/zephyrproject-rtos/zephyr/pull/55952#issuecomment-1476146602

jori-nordic commented 5 months ago

More material: https://github.com/zephyrproject-rtos/zephyr/pull/71638#discussion_r1575745662

jori-nordic commented 5 months ago

And more https://github.com/zephyrproject-rtos/zephyr/pull/71656#discussion_r1574693553

Thalley commented 4 months ago

Another: https://github.com/zephyrproject-rtos/zephyr/pull/72090#discussion_r1596897603

jori-nordic commented 4 months ago

Something to consider too: opaque data types https://github.com/zephyrproject-rtos/zephyr/pull/72090#discussion_r1598901459