Skip to content

Conversation

@kurisaW
Copy link
Member

@kurisaW kurisaW commented Sep 10, 2025

拉取/合并请求描述:(PR description)

[

Fixed: #10391

1.新增GitHub action评论脚本:pr-notify.yml,后续如有评论需求可在此基础上扩展
2.新增clang-format脚本,目前主要功能就是:当PR推送到主线,会自动触发一个评论,引导用户到自己的仓库下运行workflow,只需要一键配置排除文件或路径,以及贡献PR的分支,就可以一键运行format脚本,格式化完成后会自动追加一笔commit到当前PR下

clang-format.yml脚本功能:

  • 手动触发时可指定分支和排除文件/目录
  • 自动检查 PR 中修改的 C/C++ 源文件(.cpp, .h, .c 等)
  • 使用项目中的 .clang-format 配置文件进行格式化
  • 如果有代码变动,自动提交格式化后的代码到原分支
  • 仅检查PR修改的文件格式化内容
  • pr-notify.yml脚本仅在RT-Thread组织CI下运行;clang-format.yml脚本仅在用户fork仓库下运行

格式化情况如下:

  • 情况1:用户提供了排除模式 dir1/,且PR中有修改的文件 → 执行排除
  • 情况2:用户没有提供排除模式,但PR中有修改的文件 → 跳过排除,直接处理所有文件
  • 情况3:用户提供了排除模式,但PR中没有需要格式化的源文件 → 跳过排除,因为没有文件要处理
  • 情况4:用户没有提供排除模式,PR中也没有源文件 → 什么都不做

下面是一个实际运行效果:

image image image

为什么提交这份PR (why to submit this PR)

你的解决方案是什么 (what is your solution)

请提供验证的bsp和config (provide the config and bsp)

  • BSP:
  • .config:

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

  • 本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
  • 本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality:

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

  • 已经仔细查看过代码改动的对比 Already check the difference between PR and old code
  • 代码风格正确,包括缩进空格,命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
  • 没有垃圾代码,代码尽量精简,不包含#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up
  • 所有变更均有原因及合理的,并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
  • 对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
  • 代码是高质量的 Code in this PR is of high quality
  • 已经使用formatting 等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification
  • 如果是新增bsp, 已经添加ci检查到.github/ALL_BSP_COMPILE.json 详细请参考链接BSP自查

@github-actions github-actions bot added the action github action yml imporve label Sep 10, 2025
@github-actions
Copy link

github-actions bot commented Sep 10, 2025

📌 Code Review Assignment

🏷️ Tag: workflow

Reviewers: Rbb666 kurisaW supperthomas

Changed Files (Click to expand)
  • .github/workflows/pr_clang_format.yaml
  • .github/workflows/pr_format_bot.yml

📊 Current Review Status (Last Updated: 2025-09-23 17:36 CST)

  • Rbb666 Pending Review
  • kurisaW Pending Review
  • supperthomas Pending Review

📝 Review Instructions

  1. 维护者可以通过单击此处来刷新审查状态: 🔄 刷新状态
    Maintainers can refresh the review status by clicking here: 🔄 Refresh Status

  2. 确认审核通过后评论 LGTM/lgtm
    Comment LGTM/lgtm after confirming approval

  3. PR合并前需至少一位维护者确认
    PR must be confirmed by at least one maintainer before merging

ℹ️ 刷新CI状态操作需要具备仓库写入权限。
ℹ️ Refresh CI status operation requires repository Write permission.

@kurisaW kurisaW requested a review from Rbb666 September 10, 2025 10:20
@kurisaW
Copy link
Member Author

kurisaW commented Sep 10, 2025

@Ryan-CW-Code 大佬也可以一起帮忙看下

@Ryan-CW-Code
Copy link
Contributor

要不要增加一个clang-format检查active,发现格式不匹配的时候再提示使用这个一键格式化脚本?
排除文件要不要使用.clang-format-ignore? clang-format 18.1后应该都支持了。

@kurisaW
Copy link
Member Author

kurisaW commented Sep 10, 2025

检查脚本应该默认有一个了,其实pr里默认就是只会有一个评论,然后引导用户在自己的仓库下选择是否运行workflow,更多希望是能给贡献者带来代码检查的意识

另外对于不需要格式化的文件或目录,在workflow里已经给出配置项了,然后在执行clang format之前就删除掉希望排除的文件或目录,如果用clang的参数,那就可能还涉及到备份.clang-format,然后再插入参数,这反而是复杂化需求了

@Ryan-CW-Code
Copy link
Contributor

我想的是主仓库一个默认的.clang-format-ignore给RTT核心代码使用,然后如果用户提交的bsp部分代码不希望格式化了,就在bsp内再新建一个.clang-format-ignore给当前bsp使用。
这样的好处是官方实现的功能不用再重复实现一遍了,也可以防止后来的维护人员误格式化原作者要求不格式化的文件

@kurisaW
Copy link
Member Author

kurisaW commented Sep 10, 2025

这个建议很好,是需要这个机制的,有些厂商的源码不应该被格式化

@Ryan-CW-Code
Copy link
Contributor

目前已有的检查脚本只检查个别格式,不会对clang-format进行校验,可以在现在这个脚本前加一个clang-format --dry-run --Werror有问题继续往下走,没问题就不管了嘛

@kurisaW
Copy link
Member Author

kurisaW commented Sep 10, 2025

感谢反馈,明天我再完善下

@Ryan-CW-Code
Copy link
Contributor

感谢你的工作!
还有一点是,我个人对目前 clang-format 的格式化效果并不太满意,尤其是在 AlignConsecutiveDeclarationsAlignConsecutiveAssignments 这两项配置下,代码会被对齐成这样:

    struct rt_serial_device  *serial;
    struct rt_serial_tx_fifo *tx_fifo        = RT_NULL;
    rt_err_t                  control_result = RT_EOK;

这种对齐方式在我看来很别扭。

代码的规范还需要您们再完善一下 documentation/7.contribution/coding_style_cn.md

@kurisaW
Copy link
Member Author

kurisaW commented Sep 10, 2025

感谢你的工作! 还有一点是,我个人对目前 clang-format 的格式化效果并不太满意,尤其是在 AlignConsecutiveDeclarationsAlignConsecutiveAssignments 这两项配置下,代码会被对齐成这样:

    struct rt_serial_device  *serial;
    struct rt_serial_tx_fifo *tx_fifo        = RT_NULL;
    rt_err_t                  control_result = RT_EOK;

这种对齐方式在我看来很别扭。

代码的规范还需要您们再完善一下 documentation/7.contribution/coding_style_cn.md

可以试试这个?
image

效果的话详见下图,个人认为如果要综合考虑其他格式化情况,这里使用非对齐会好些:
image

@Ryan-CW-Code
Copy link
Contributor

对这样是可以,主要是我不清除您们那边的规范,也就不敢随意修改clang-format的配置

@kurisaW
Copy link
Member Author

kurisaW commented Sep 10, 2025

对这样是可以,主要是我不清除您们那边的规范,也就不敢随意修改clang-format的配置

其实我们在代码格式化这块做的也不够完善,格式化标准还需要进一步扩展详细说明,当然格式化工具引入后,也希望大家多多反馈格式化问题,以便我们进一步优化

@kurisaW
Copy link
Member Author

kurisaW commented Sep 10, 2025

@Ryan-CW-Code 这里提供了一个版本,两套机制来控制排除格式化构建:

  • 第一是workflow自带的exclude_patterns,这里由用户手动指定;
  • 第二是有原本仓库下的.ignore_format.yml,支持对PR修改文件的当前向上和向下递归检索.ignore_format.yml规则。

这两套机制会分别走一次,最终将真正需要格式化的文件列出并进行格式化:

image

但是这里还存在一个问题,由于workflow是手动触发的,他并不具备PR的上下文,也就是说,他只能获取到最近一次commit的PR文件的全部变更信息,也就是说提交一次commit最好就触发一次workflow,这个机制可能还需要再研究看看

@kurisaW kurisaW marked this pull request as draft September 11, 2025 03:26
@Ryan-CW-Code
Copy link
Contributor

Ryan-CW-Code commented Sep 11, 2025

同步一下

.clang-format 用于配置代码格式化规则,支持向上递归查找配置文件。通过设置 BasedOnStyle: InheritParentConfig,可继承并合并父目录中的配置,实现灵活的层级化管理。建议将该文件置于项目根目录以统一风格,也可在子目录中添加局部配置以覆盖上级规则。

.clang-format-ignore 用于指定不参与格式化的文件或目录,同样支持向上递归查找。需要注意的是,目前不支持多个忽略文件的合并处理,仅采用最近找到的那个 .clang-format-ignore 文件内容,需要确保父目录和子目录.clang-format-ignore文件之间的冲突,可以通过合理设置.clang-format-ignore来避免。

@kurisaW
Copy link
Member Author

kurisaW commented Sep 11, 2025

总结一下:

  • 用户提交PR后,可根据评论提示直达workflow页面,配置排除内容、分支以及PR号(评论中会有提示)
  • 用户手动执行workflow后,会自动生成一条format commit,其中排除规则如下(默认两套机制):
    • 1.仓库目录下存在的.clang-format-ignore文件,该机制为clang-format v18之后默认支持的排除格式化机制;
    • 2.workflow中的exclude_patterns参数,可排除指定的目录和文件
  • 仓库下默认的.clang-format-ignore机制优先级高于workflow中的exclude_patterns参数
  • 支持自动检索PR内所有修改的文件list

@kurisaW kurisaW marked this pull request as ready for review September 14, 2025 05:27
@Rbb666 Rbb666 requested a review from unicornx September 23, 2025 02:22
@Rbb666 Rbb666 merged commit 4123daf into RT-Thread:master Sep 23, 2025
64 checks passed
@Rbb666 Rbb666 linked an issue Oct 20, 2025 that may be closed by this pull request
@kurisaW kurisaW deleted the astyle branch November 14, 2025 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action github action yml imporve

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] 推动clang-format-ignore机制 [Feature] 对代码格式化工具进行确认

3 participants