Skip to content

Conversation

tianshuo78520a
Copy link
Collaborator

@tianshuo78520a tianshuo78520a commented Mar 3, 2022

add code review

@paddle-bot-old
Copy link

paddle-bot-old bot commented Mar 3, 2022

Thanks for your contribution!

Copy link
Collaborator

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. dev_guides目录下需要新建一个git_guides,来放这里的所有文档。
  2. 有两个链接(编译和CI)需要等对应文档上传后,更新下链接
  3. 英文对应修改


```bash
➜ pip install pre-commit
➜ pip install pre-commit==1.10.4
➜ pre-commit install
```

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. clang-format要明确下是3.8版本,不是3.8以上
  2. 编译和单元测试,去掉单元测试。
  3. (TODO)编译要跳到 @pangyoki 新的链接里,可以等目录做完后,再次调整。

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好的,已经修改

## 建立 Issue 并完成 Pull Request

建立一个 Issue 描述问题,并记录它的编号。
## 完成 Pull Request PR创建
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
这张图可以裁剪掉一些分支么,太大了。

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好的

@@ -16,6 +14,10 @@

接下来等待 review,如果有需要修改的地方,参照上述步骤更新 origin 中的对应分支即可。

### 关联Issue

如果解决了某个Issue的问题创建的PR,需要关联Issue,参考[Code Reivew 约定](./code_review_cn.html)
Copy link
Collaborator

@luotao1 luotao1 Mar 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

19行可以合并到13行,都是关联issue,可以修改一下话术。去掉17-19行

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

话术合并后改成:
如果解决了某个Issue的问题创建的PR,需要关联Issue,参考Code Reivew 约定。并在 PR 的描述说明中,填写 resolve #Issue编号 可以在这个 PR 被 merge 后,自动关闭对应的 Issue,具体请见[这里]

@@ -45,9 +47,7 @@

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

46行的【提交commit】链接,链接过去的不对。

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -45,9 +47,7 @@

请您关注您Pull Request中的CI单元测试进程,它将会在几个小时内完成
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO:等 @lelelelelez 的CI文章merge后,这里加一下链接

@@ -0,0 +1,33 @@
# Code Review 约定

## 提交代码的一些约定
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不需要第三行的小标题了

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Collaborator

@TCChenlong TCChenlong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

全文 “您” -> 你


当所需的测试后都出现了绿色的对勾,表示您本次commit通过了各项单元测试,您只需要关注显示Required任务,不显示的可能是我们正在测试的任务

如果所需的测试后出现了红色叉号,代表您本次的commit未通过某项单元测试,在这种情况下,请您点击detail查看报错详情,并将报错原因截图,以评论的方式添加在您的Pull Request中,我们的工作人员将帮您查看
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里应该是优先开发者自行解决吧?无法解决的时候再评论到评论区,一起看下

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好的,已经修改


## 删除远程分支

在 PR 被 merge 进主仓库后,我们可以在 PR 的页面删除远程仓库的分支。
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我们 -> 你


### 签署CLA

在首次向PaddlePaddle提交Pull Request时,您需要您签署一次CLA(Contributor License Agreement)协议,以保证您的代码可以被合入,具体签署方式如下:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

您需要您签署一次CLA -> 你需要签署一次CLA

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


![change_base](../guides/10_contribution/img/change_base.png)

如果解决了某个Issue的问题创建的 PR,需要关联Issue,参考[Code Reivew 约定](./code_review_cn.html)。并在 PR 的描述说明中,填写 `resolve #Issue编号` 可以在这个 PR 被 merge 后,自动关闭对应的 Issue,具体请见[这里](https://help.github.com/articles/closing-issues-via-commit-messages/)。
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

复制这段吧:如果解决了某个Issue的问题,请在该PUll Request的第一个评论框中加上:fix #issue_number,这样当该PUll Request被合并后,会自动关闭对应的Issue。关键词包括:close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved,请选择合适的词汇。详细可参考Closing issues via commit messages


选择目标分支:

![change_base](../guides/10_contribution/img/change_base.png)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

两张图都没有预览出来,可以删掉原来的图片,在git_guides目录下放上最新图片。

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好的


在 PR 被 merge 进主仓库后,我们可以在 PR 的页面删除远程仓库的分支。

![delete_branch](../guides/10_contribution/img/delete_branch.png)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

图片没有预览出来

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好的,之前添加了git_guides目录,导致路径不对了,已经修改

Copy link
Collaborator

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

![change_base](../../guides/10_contribution/img/change_base.png)

如果解决了某个Issue的问题创建的 PR,需要关联Issue,参考[Code Reivew 约定](./code_review_cn.html)。并在 PR 的描述说明中,填写 `resolve #Issue编号` 可以在这个 PR 被 merge 后,自动关闭对应的 Issue,具体请见[这里](https://help.github.com/articles/closing-issues-via-commit-messages/)。
如果解决了某个Issue的问题,请在该PUll Request的第一个评论框中加上:fix #issue_number,这样当该PUll Request被合并后,会自动关闭对应的Issue。关键词包括:close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved,请选择合适的词汇。详细可参考[Closing issues via commit messages](https://help.github.com/articles/closing-issues-via-commit-messages/)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

13行去掉,保留14行即可

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


切换到所建分支,然后点击 `New pull request`。

![new_pull_request](../../guides/10_contribution/img/new_pull_request.png)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

目前这个图和原来的图比,有点模糊

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New pull request有些老,已经找不到,改成了Compare & pull request

Copy link
Collaborator

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

LGTM 使用指南里的参与开发,请 @TCChenlong 统一调整

Copy link
Collaborator

@TCChenlong TCChenlong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TCChenlong TCChenlong merged commit d06c389 into PaddlePaddle:develop Mar 8, 2022
RichardWooSJTU pushed a commit to RichardWooSJTU/docs that referenced this pull request Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants