gaogaotiantian / biliscope

Bilibili chrome extension to show uploader's stats
MIT License
575 stars 45 forks source link

Add pre-commit ci and CONTRIBUTING.md #127

Closed F-park closed 8 months ago

F-park commented 8 months ago
gaogaotiantian commented 8 months ago

我想象的lint应该是一个js级别的lint,这个linter应该有你用的这些github action的功能。现在这个linter的功能过于有限了,而且是不可扩展的(它不可能支持js级别的lint),而未来如果有js的lint,肯定能发现这种trailing space的问题。

F-park commented 8 months ago

现在这个linter的功能过于有限了,而且是不可扩展的(它不可能支持js级别的lint)。

可以扩展的,可以加上 eslint 或 jslint 的 pre-commit hook,只是我不知道怎样的 lint 规则才算好,不知道是否可以参考 viztracer-vscode 里的规则(但那个是 nodejs 项目)


这里可以查到 JavaScript 的 hook。

gaogaotiantian commented 8 months ago

Hmm,你这里用的是pre commit hooks是吧,这里不应该是做修复的。这里需要一个PR时候运行的action,然后去做一个lint,这个linter不应该是一个纯的别人写好的action,应该是我们这边可以configure的一个东西,参考一下viztracer的lint那个workflow。javascript这边我不是很确定,eslint?应该是一个我们在命令行可以独立运行的东西,而不是只有PR的时候pre-commit才跑。

F-park commented 8 months ago

应该是一个我们在命令行可以独立运行的东西,而不是只有PR的时候pre-commit才跑。

主要是保证不了所有贡献者都在本地使用 pre-commit,还是得用 github actions 强制检查一遍。(这个 CI 的目的就在这里)


使用自动修复的原因是减少处理流程(不能保证所有人会看 github actions 的日志)

F-park commented 8 months ago

这里需要一个PR时候运行的action,然后去做一个lint,这个linter不应该是一个纯的别人写好的action,应该是我们这边可以configure的一个东西。

lint 新开一个 action 也可以,不过这个其实可以后面再加吧(主要是得找到一个合适的 configure)


eslint 带有 format 功能(应该是可以通过 configure 开启的),如果贡献者本地没有安装 eslint 的话,也可以使用 github actions 进行自动修复。

gaogaotiantian commented 8 months ago

我重新看了一下,pre-commit相当于是个库是吧。

首先,自动修复这个事情一定是拿掉的,这是一个可能产生的问题多于解决的问题的东西。

github action当然是要有的,但是本地也应该有一样的解决方案——保证在本地运行的lint结果和在github action上一样,这就是为什么我建议你在github action上直接用eslint。

你现在的这个action就是要做lint,为啥要再开个action做lint呢?现在加的这个ci,就是要用一个linter检查代码。当你用了eslint,你现在写的很多东西就要扔掉了,比如查trailing space的,因为eslint也可以做。就这个中间步骤是没啥意义的,搞明白了eslint的部分就直接上eslint就好了啊。

contributing的这个md file现在可以不加,以后再考虑。如果你需要有coding style,那一定是有一个linter背后做支撑的,而不是写几行“代码应该怎么写”。所有的coding style preference都体现在你linter的configuration里。

就这个PR只需要让eslint在github action上跑起来,同时能在本地操作正确的情况下(npm lint之类的)可以跑出来同样的结果。中间步骤价值太小了。

F-park commented 8 months ago

当你用了eslint,你现在写的很多东西就要扔掉了,比如查trailing space的,因为eslint也可以做。就这个中间步骤是没啥意义的,搞明白了eslint的部分就直接上eslint就好了啊。

eslint 只能检测和格式化 JavaScript 文件,但 pre-commit 这个 hook 可以检测所有文件,我觉得是有必要区分 js 的 lint 和其他文件的 lint 的。

gaogaotiantian commented 8 months ago

你在做一个特别边缘的事情。就是说,你引入的这个整个东西,现在只做了一件事——查空格。这个事情它本身价值是偏低的。

我们做项目,一定是解决主要矛盾,在主要矛盾存在的时候,不要总是去找边角的事情。比如去做一个contributing的文档。项目几乎没有contributor,做它干啥?

现在什么是主要矛盾?去做一个JavaScript level的lint。你说eslint没法处理其他格式,没错,但是还有多少其他格式要处理?这是一个JavaScript的项目啊。这就像你画画,老师让你画虾,你虾没怎么画呢,然后老去想水面上的浮萍应该朝着哪个方向,它不重要啊!

每一行新的代码,不管是feature还是CI,都会引入额外的maintain burden,在项目本身还在发展期的情况下,一个极大的考量是,我引入的这个maintain的cost,是不是值得。它带来了多少好处,又带来了多少麻烦。比如,你现在引入了一个pre-commit库,于是在local如果想用,你需要python环境。问题这是一个JavaScript的项目,你相当于在开发阶段又多了一个完全其他语言的dependency,这是一个非常不理想的事。而引入一个完全和项目无关的语言为了解决什么问题呢——找空格。严格来说,是找非JavaScript代码的空格,因为JavaScript的可以通过eslint解决。这在我看来是难以接受的。

我个人的真诚建议是,把目光放在“有价值”的事上。low hanging fruit是非常诱人的,但是在项目发展的初期,有大量valuable的low hanging fruit。做一个事情,不应该是“我可以”,而应该是“有意义”。这个有意义,不是说只要带来了好处,就有意义,而是带来的好处要比引入的麻烦明显多。

就比如之前我反对的那个用global variable去传当前用户名,它就明显让程序结构更复杂了。新的feature经常是好事,但是一定要去管cost,因为一个feature不是写完了好像工作了就完了,后面还有maintain的事情。

现阶段我觉得提升最大的,性价比最高的,就是我之前提到的——lint和test。因为在项目长大的过程中,没有这两个东西,你是不敢随便check in代码的,太容易break了。

F-park commented 8 months ago

懂了,那就关闭这个 pr 先完成应该做的事情吧