Skip to content
This repository has been archived by the owner on Jan 29, 2023. It is now read-only.

adjust github pre-commit action #510

Closed
wants to merge 4 commits into from

Conversation

huazhouwang
Copy link

@huazhouwang huazhouwang commented Feb 23, 2021

What does this implement/fix? Explain your changes.

调整 github pre-commit action 逻辑 (test only)

Does this close any currently open issues?

If it fixes a bug or resolves a feature request, be sure to link to that issue.

Pull request type

Put an x in the boxes that apply

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Where has this been tested?

Any other comments?

@huazhouwang
Copy link
Author

@danielsocials 目前找不到更好获得 PR 变更文件的方式,所以我想既然咱们决定用 pre-commit 的话,是否可以先全局格式化,那么后面每个 PR 都只需要执行默认的 ‘pre-commit run --all-files’ 就好

@huazhouwang huazhouwang deleted the pre-commmit branch February 23, 2021 12:10
@taimanhui
Copy link

python部分,black和isort需要保持跳过electrum,原因是还是希望能跟到上游,避免和上游diff的时候会有太多的字符改动,尤其是我们的行宽用到了120而非常用的79,貌似black还会依据这个进行修改。

不过你也没改isort和black的配置,貌似这里面的改动都是针对无谓空格进行处理,而且貌似很多是前期改动遗留下来的,我稍微看了下应该没我上面说的问题。

不过除了用来在github workflow里做PR的检查,我不建议用pre-commit run --all-files,原因和不应该用git add .git commit -a一样。而且本地用也不是对整个PR的,而应该是每一个commit都要检查,如它的名字这个其实应该是个pre commit hook,每次commit的时候顺便做检查和格式化用的。

@huazhouwang
Copy link
Author

目前仅检查变更文件的方式会遇到这个问题,不确定怎么解决合适。参考:actions/checkout#27

@huazhouwang
Copy link
Author

仅讨论:
其实目前咱们代码已经跟上游差别很大,即使后面需要参考上游代码也只能逐行 diff 形式变更。与其担心这个,我反倒觉得项目保持同个风格会更好

@taimanhui
Copy link

仅讨论:
其实目前咱们代码已经跟上游差别很大,即使后面需要参考上游代码也只能逐行 diff 形式变更。与其担心这个,我反倒觉得项目保持同个风格会更好

其实还好,而且这里面还不乏非同一行的空格改动,有单元测试之后是可以跟得上的,尽量把electrum变回个库用,就算以后不用electrum了也能知道需要实现什么内容即可。

git diff upstream/master --stat --diff-filter=a -b -- electrum ':!electrum/gui' ':!electrum/locale'
...
113 files changed, 3734 insertions(+), 7191 deletions(-)

其实想要保持一致问题也不大,把我们默认行宽改回79,然后避免black做那种单双引号的转换就可以了,其他的black就算自动修改也还是能接受的,但是行宽和引号这两个问题打击面太大了。

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants