You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@mynewt.apache.org by "szymon-czapracki (via GitHub)" <gi...@apache.org> on 2026/10/09 10:44:22 UTC
[PR] Mynewt format [mynewt-core]
szymon-czapracki opened a new pull request, #3761:
URL: https://github.com/apache/mynewt-core/pull/3761
## Why
Code style is enforced today with clang-format, which has not served the project well:
- **It is an external tool we do not control.** Its output depends on the clang-format
version installed, so the same code can pass locally and fail in CI.
- **It rewrites correct code.** It reformats whole statements instead of fixing what is
wrong, so deliberate layout (aligned `#define` columns, hand-made line breaks, tables)
gets rewritten.
- **Its options cannot express our standard.** Much of `CODING_STANDARDS.md` (mandatory
braces, include guards, license headers, declarations at the top of functions, naming,
`//` comments) is outside what clang-format can check, and its settings cannot be
tuned to the project's own rules.
This PR adds `mynewt_format`, a style checker the project owns. It runs **next to**
clang-format as an informational check, so its rules can be judged on real pull requests
before anyone relies on them.
## What it does
- **Fixes problems, never reformats.** Each rule touches only the text that breaks it;
intentional layout is kept.
- **Safe fixes.** Every fix is checked against the file's token stream (preprocessor
line structure included); a fix that would change what the compiler sees is
discarded.
- **Checks only changed lines by default**, so existing code that predates a rule does
not get in the way (`--whole-files` checks everything).
- **Modes:** report (default), `-d` patch, `-f` apply fixes, `-i` review fixes one by one.
- **Readable reports:** every problem is shown with its source line and the proposed
fix right below it, so a CI log can be read without opening the files;
`--report list` gives one line per problem for editors and scripts.
- **Self-documenting:** `--list-rules` lists the rules, `--explain RULE` explains one.
- Pure Python 3.11+, standard library only. Rules are small plugins in
`tools/mynewt_style/rules/`, configured per repository in `newt-coding-rules`.
- Honours inline suppressions (`/* mynewt-format: ignore */`, `off`/`on`) and existing
`/* clang-format off */` regions.
## Changes
- `tools/mynewt_format.py`, `tools/mynewt_style/`: engine, rules, unit tests and
documentation. This is the canonical copy; apache-mynewt-nimble syncs it from here.
- `tools/mynewt_style/CODING_STANDARDS.md`: the coding standard the tool checks, section
by section, with examples and the rule behind each guideline.
- `newt-coding-rules`: configuration (80 column warning, 100 column error, rulesets).
- `newt-coding-rules-ignore`: everything in `.clang-format-ignore`, the third-party files
listed in `.rat-excludes`, vendored SDK sources (`ext/`, `vendor/`) and linker script
headers (`*.ld.h`).
- `.github/workflows/check_compliance.yml`: new job **"Coding style (mynewt_format,
informational)"**. It checks the lines changed by the PR, shows each problem with its
code and proposed fix, prints a patch with all fixes, and **never fails the build**.
## Not changed
- The clang-format CI check, `.clang-format` and `.clang-format-ignore`: still the
required style check.
- `CODING_STANDARDS.md` at the repository root: still authoritative where it differs
from the new document.
## For reviewers to decide
- **Return type placement.** mynewt_format puts the return type on the same line as the
function name; clang-format and the current `CODING_STANDARDS.md` put it on its own
line. Until this is settled the informational job reports clang-formatted
definitions. It is a single option (`[rules.return-type-placement] style`).
- **Line length.** 80 columns as a target and 100 as the hard limit, instead of 79.
- **Function name prefixes** are not configured for core yet (only the lower case check
applies); per-directory prefixes can be added in `newt-coding-rules`.
## Validation
- 81 unit tests: every rule is tested for what it must report and what it must leave
alone, every fix for clean, idempotent output.
- Run over the whole repository: no fix changes code tokens, a second run changes
nothing, and every fixable problem is fixed.
- Checked against all lines committed in the last year to weed out false positives.
## Try it
python3 tools/mynewt_format.py --git-diff origin/master # lines changed on your branch
python3 tools/mynewt_format.py -i # review fixes one by one
python3 tools/mynewt_format.py --list-rules
python3 -m unittest discover -s tools/mynewt_style/tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "szymon-czapracki (via GitHub)" <gi...@apache.org>.
szymon-czapracki commented on code in PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761#discussion_r4348978579
##########
tools/mynewt-format.py:
##########
@@ -0,0 +1,38 @@
+#!/usr/bin/env python3
Review Comment:
Renamed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "szymon-czapracki (via GitHub) via notifications" <no...@mynewt.apache.org>.
szymon-czapracki commented on PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761#issuecomment-6662282389
Pushed a few CI changes:
- mynewt-format CI is now done with composite actions (`.github/actions/mynewt-format-check` and `mynewt-format-comments`) instead of reusable workflows, so it matches the rest of `.github`. The comment posting logic moved from inline JS to `.github/scripts/mynewt_format_comments.py`, same way as `pr_maintenance.py`. NimBLE can still use it via `apache/mynewt-core/.github/actions/...@master`.
- The mynewt-format job now checks out the PR head instead of the merge commit. Before, if master changed the same file above the PR's changes, review comments could end up on the wrong lines.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "sjanc (via GitHub)" <gi...@apache.org>.
sjanc commented on code in PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761#discussion_r4327849731
##########
tools/mynewt_format.py:
##########
@@ -0,0 +1,38 @@
+#!/usr/bin/env python3
Review Comment:
mynewt-format.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "sjanc (via GitHub)" <gi...@apache.org>.
sjanc commented on PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761#issuecomment-6508250626
looks better now :) few comments though
1) remove the license header check/fix from tools (this is RAT job)
2) cpp comment fix should first check if there would be no nested comments and if so, move it to new line:
```
// foo() /* foo */
should be fixed to
/* foo */
/* foo() */
```
3) regarding line lengths, it should follow this : line length 80 is preferred, up to 100 is allowed when the author chose it (ie explicitly make it 81-99 long, if line is >100 is shall be fixed to 80
line-length.report_target = false and unnecessary-wrap.max = 80 should do it I think (also update coding style with this clarification)
4) ignore list needs some tuning,
those should be added (as this is imported code we don't wanna change much), if this list is not too long (ie not hundreds) I'd prefer to have those explicit per file (cmsis_nvic.h can match all since we have multiple copies), also "ext" folders if they have only imported code could be group matched (explicit folders, not all matching ext name)
```
fs/fatfs
hw/drivers/lora/sx127x
LoRaMac headers
sys/queue.h
Ambiq apollo2 headers
cmsis_nvic.h
nordic system_nrf5340/9160/54
util/coremark
display cfb/ssd1673
```
kernel/os/src/os_mbuf.c should be removed from ignore,
also baselibc should be removed (this is strictly speaking imported code but it is heavily modfied and mainly maintaned here, we can make it follow our style)
5) assign-in-condition flags common `while (x = foo())` pattern too; add explicit assign-in-loop option to tune this, for now we are not forbiding this pattern (yet)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "szymon-czapracki (via GitHub)" <gi...@apache.org>.
szymon-czapracki commented on code in PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761#discussion_r4348987219
##########
tools/README.md:
##########
@@ -0,0 +1,84 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+-->
+
+# Apache Mynewt Core Tools
+
+- **`mynewt_format.py`**: coding style checker and fixer. Configured by
+ [`newt-coding-rules`](../newt-coding-rules); files to skip are listed in
+ [`newt-coding-rules-ignore`](../newt-coding-rules-ignore).
+- **`mynewt_style/`**: the engine, its rules (`mynewt_style/rules/`, see the
+ [rules README](mynewt_style/rules/README.md)) and unit tests.
+- **[`mynewt_style/CODING_STANDARDS.md`](mynewt_style/CODING_STANDARDS.md)**: the
+ coding standard the tool checks, rule by rule.
+
+This is the canonical copy of the tool. apache-mynewt-nimble carries an
Review Comment:
Ok, readme says that the tool lives here in core. The discussion about integrating this with newt will happen in the future :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "szymon-czapracki (via GitHub)" <gi...@apache.org>.
szymon-czapracki commented on code in PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761#discussion_r4348970337
##########
.github/workflows/check_compliance.yml:
##########
@@ -86,6 +86,37 @@ jobs:
if: steps.label_check.outputs.skip == 'true'
run: echo "Skipping style check because 'skip-style-check' label is present."
+ style_check_mynewt_format:
+ name: Coding style (mynewt_format, informational)
+ runs-on: ubuntu-24.04
Review Comment:
Workflows run now on ubuntu-slim.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "szymon-czapracki (via GitHub) via notifications" <no...@mynewt.apache.org>.
szymon-czapracki commented on PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761#issuecomment-6605014148
> Hi,
>
> 1 thing I'd like to get fixed before this goes in
>
> doxygen comments: when moving to new line (if too long or removing C++ comments) those should be adjusted to reclect that those no longer comment previous item
>
> ```
> //foo(); /**< foobar */
> ```
>
> should become
>
> ```
> /* foobar */
> /* foo(); */
> ```
>
> That holds for every comment this rule moves out of a // line ( /**<, /*!<, /**, /*! )
Doxygen markers (/**<, /*!<, /**, /*!) are now dropped whenever a comment gets moved to its own line, so your example becomes:
/* foobar */
/* foo(); */
While testing it I also found that x = 1; //foo(); /**< foobar */ didn't get fixed at all. comment-placement ran first, produced a nested comment, and the fix was discarded. That's fixed now too, and it ends up as:
/* foobar */
/* foo(); */
x = 1;
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "szymon-czapracki (via GitHub) via notifications" <no...@mynewt.apache.org>.
szymon-czapracki commented on PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761#issuecomment-6663569976
One more change: if a PR has more than 50 new mynewt-format problems, the bot no longer posts 50 inline comments. Instead it leaves one comment suggesting to run `mynewt-format -i --git-diff origin/master` locally. On later pushes it updates that same comment instead of adding new ones.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "sjanc (via GitHub)" <gi...@apache.org>.
sjanc commented on code in PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761#discussion_r4326580266
##########
.github/workflows/check_compliance.yml:
##########
@@ -86,6 +86,37 @@ jobs:
if: steps.label_check.outputs.skip == 'true'
run: echo "Skipping style check because 'skip-style-check' label is present."
+ style_check_mynewt_format:
+ name: Coding style (mynewt_format, informational)
+ runs-on: ubuntu-24.04
Review Comment:
ubunut-slim (should be enough for code style)
##########
newt-coding-rules:
##########
@@ -0,0 +1,77 @@
+#
Review Comment:
I'd make those config files hidden and named after tool name (ie .mynewt-format and .mynewt-format-ignore)
##########
.github/workflows/check_compliance.yml:
##########
@@ -86,6 +86,37 @@ jobs:
if: steps.label_check.outputs.skip == 'true'
run: echo "Skipping style check because 'skip-style-check' label is present."
+ style_check_mynewt_format:
+ name: Coding style (mynewt_format, informational)
+ runs-on: ubuntu-24.04
+ # Informational only: reports problems but never fails the PR.
+ continue-on-error: true
+ steps:
+ - name: Checkout code
+ uses: actions/checkout@v4
+ with:
+ fetch-depth: 0 # Required to get full history for diff
+
+ - name: Set up Python
Review Comment:
could be updated to latest action version, also use 3.x version, no need to pin to 3.12
##########
tools/README.md:
##########
@@ -0,0 +1,84 @@
+<!--
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+-->
+
+# Apache Mynewt Core Tools
+
+- **`mynewt_format.py`**: coding style checker and fixer. Configured by
+ [`newt-coding-rules`](../newt-coding-rules); files to skip are listed in
+ [`newt-coding-rules-ignore`](../newt-coding-rules-ignore).
+- **`mynewt_style/`**: the engine, its rules (`mynewt_style/rules/`, see the
+ [rules README](mynewt_style/rules/README.md)) and unit tests.
+- **[`mynewt_style/CODING_STANDARDS.md`](mynewt_style/CODING_STANDARDS.md)**: the
+ coding standard the tool checks, rule by rule.
+
+This is the canonical copy of the tool. apache-mynewt-nimble carries an
Review Comment:
nimble should not have own copy, both user and CI can use one from core (in long term we could even think on integrating this with newt hm?)
##########
tools/mynewt_format.py:
##########
@@ -0,0 +1,38 @@
+#!/usr/bin/env python3
Review Comment:
mynewt-format
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "sjanc (via GitHub) via notifications" <no...@mynewt.apache.org>.
sjanc commented on PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761#issuecomment-6601418726
Hi,
1 thing I'd like to get fixed before this goes in
doxygen comments: when moving to new line (if too long or removing C++ comments) those should be adjusted to reclect that those no longer comment previous item
```
//foo(); /**< foobar */
```
should become
```
/* foobar */
/* foo(); */
```
That holds for every comment this rule moves out of a // line ( /**<, /*!<, /**, /*! )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "szymon-czapracki (via GitHub)" <gi...@apache.org>.
szymon-czapracki commented on code in PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761#discussion_r4348976289
##########
.mynewt-format:
##########
@@ -0,0 +1,77 @@
+#
Review Comment:
It's hidden now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "sjanc (via GitHub) via notifications" <no...@mynewt.apache.org>.
sjanc merged PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "szymon-czapracki (via GitHub)" <gi...@apache.org>.
szymon-czapracki commented on PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761#issuecomment-6523713001
> looks better now :) few comments though
>
> 1. remove the license header check/fix from tools (this is RAT job)
> 2. cpp comment fix should first check if there would be no nested comments and if so, move it to new line:
>
> ```
> // foo() /* foo */
>
> should be fixed to
> /* foo */
> /* foo() */
> ```
>
> 3. regarding line lengths, it should follow this : line length 80 is preferred, up to 100 is allowed when the author chose it (ie explicitly make it 81-99 long, if line is >100 is shall be fixed to 80
> line-length.report_target = false and unnecessary-wrap.max = 80 should do it I think (also update coding style with this clarification)
> 4. ignore list needs some tuning,
> those should be added (as this is imported code we don't wanna change much), if this list is not too long (ie not hundreds) I'd prefer to have those explicit per file (cmsis_nvic.h can match all since we have multiple copies), also "ext" folders if they have only imported code could be group matched (explicit folders, not all matching ext name)
>
> ```
> fs/fatfs
> hw/drivers/lora/sx127x
> LoRaMac headers
> sys/queue.h
> Ambiq apollo2 headers
> cmsis_nvic.h
> nordic system_nrf5340/9160/54
> util/coremark
> display cfb/ssd1673
> ```
>
> kernel/os/src/os_mbuf.c should be removed from ignore, also baselibc should be removed (this is strictly speaking imported code but it is heavily modfied and mainly maintaned here, we can make it follow our style)
>
> 5. assign-in-condition flags common `while (x = foo())` pattern too; add explicit assign-in-loop option to tune this, for now we are not forbiding this pattern (yet)
1. License header check is gone - RAT will take care of that.
2. // comments with a /* */ inside now get the inner comment moved to its own line first, so:
// foo() /* foo */ turns into /* foo */ and /* foo() */ below it.
3. Went with your config suggestion (report_target = false, unnecessary-wrap.max = 80) and updated the coding style doc. I've changed the fix itself, before it wrapped long lines to 100, now it wraps them back to 80.
4. Added the ignore files from your list (among others), one per line.
Removed os_mbuf.c and baselibc.
Applied your comment about ext folder and cmsis.
5. Added an allow_assign_in_loop option, on by default, so while ((x = foo())) is fine for now and only if gets checked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduction of new custom code formatting tool [mynewt-core]
Posted by "szymon-czapracki (via GitHub)" <gi...@apache.org>.
szymon-czapracki commented on code in PR #3761:
URL: https://github.com/apache/mynewt-core/pull/3761#discussion_r4348972792
##########
.github/workflows/check_compliance.yml:
##########
@@ -86,6 +86,37 @@ jobs:
if: steps.label_check.outputs.skip == 'true'
run: echo "Skipping style check because 'skip-style-check' label is present."
+ style_check_mynewt_format:
+ name: Coding style (mynewt_format, informational)
+ runs-on: ubuntu-24.04
+ # Informational only: reports problems but never fails the PR.
+ continue-on-error: true
+ steps:
+ - name: Checkout code
+ uses: actions/checkout@v4
+ with:
+ fetch-depth: 0 # Required to get full history for diff
+
+ - name: Set up Python
Review Comment:
Changed to 3.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org