Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

为allMagicNB合并版本的构建文件重新添加注释 #4025

Closed
wants to merge 15 commits into from

Conversation

Silverteal
Copy link
Contributor

@Silverteal Silverteal commented Jun 14, 2024

相关:#4009

@Silverteal Silverteal marked this pull request as draft June 14, 2024 06:25
@Silverteal Silverteal changed the title Patch 2 为AllMagicNB合并版本的构建文件重新添加注释 Jun 14, 2024
@Silverteal Silverteal changed the title 为AllMagicNB合并版本的构建文件重新添加注释 为allMagicNB合并版本的构建文件重新添加注释 Jun 14, 2024
@3gf8jv4dv
Copy link
Collaborator

Do you mean to re-add the comment based on #4009?

If this is the case, you may need to create a new branch based on the current Hex-Dragon:main branch, or recreate the commits after pulling from the Hex-Dragon:main branch to the Silverteal:patch-2 branch.

@Silverteal
Copy link
Contributor Author

Do you mean to re-add the comment based on #4009?

If this is the case, you may need to create a new branch based on the current Hex-Dragon:main branch, or recreate the commits after pulling from the Hex-Dragon:main branch to the Silverteal:patch-2 branch.

I will.

@Silverteal Silverteal marked this pull request as ready for review June 14, 2024 15:46
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@allMagicNB
Copy link
Contributor

我反对此 PR。

首先,我认为没有必要为五十行左右的代码添加三十行左右的注释,并且部分注释描述并不准确。也没有必要将工作流汉化,一般需要看这些的人都能看懂这些简单的英语,看不懂的也基本不需要看懂,翻译器做不到完美翻译,使用英语也更加国际化,利于国际友人理解,因为目前英语还是世界上使用最广泛的语言。
其次,引号写法很正常,一般一眼都能看懂,看不懂的也用不着看懂,并没有必要加个变量传入参数。
再者,获取 Describe 出错也是因为没找到 Tag 才出错的,但这在主仓库基本不会发生,因此此步骤我认为没有必要。
至于 Artifact 的命名,我并不理解为何要加 pcl2-autobuild,一般来说看后面的 Describe 部分比一般长就能看出来这并不是普通版本。

@Silverteal
Copy link
Contributor Author

Silverteal commented Jun 14, 2024

首先,我认为没有必要为五十行左右的代码添加三十行左右的注释

好的。

并且部分注释描述并不准确。

如果不准确,那应该修改,而不是反对。

也没有必要将工作流汉化,一般需要看这些的人都能看懂这些简单的英语,看不懂的也基本不需要看懂,翻译器做不到完美翻译,使用英语也更加国际化,利于国际友人理解,因为目前英语还是世界上使用最广泛的语言。

纵使存在国际友人的参与,依然不能改变其完全面向中文使用者的事实。
如果这方面有多于1人的负面反馈,我稍后再重新考虑。

其次,引号写法很正常,一般一眼都能看懂,看不懂的也用不着看懂,并没有必要加个变量传入参数。

这是对的。已经修正。

再者,获取 Describe 出错也是因为没找到 Tag 才出错的,但这在主仓库基本不会发生,因此此步骤我认为没有必要。

其他仓库会发生,保留也不会对本仓库有负面影响。

至于 Artifact 的命名,我并不理解为何要加 pcl2-autobuild,一般来说看后面的 Describe 部分比一般长就能看出来这并不是普通版本。

版本号前增加产品名称应该是很常见的实践。把构建的压缩文件发到任意一个开发群都可以一眼看出这是什么程序,从哪里来的。

@allMagicNB
Copy link
Contributor

再者,获取 Describe 出错也是因为没找到 Tag 才出错的,但这在主仓库基本不会发生,因此此步骤我认为没有必要。

其他仓库会发生,保留也不会对本仓库有负面影响。

那是异常的仓库,我这边就很正常,这种错误不应该为他们擦屁股,这通常是他们自己导致的。

版本号前增加产品名称应该是很常见的。把构建的压缩文件发到任意一个开发群都可以一眼看出这是什么程序,从哪里来的。

Screenshot_20240615_071450_com.microsoft.emmx~2.jpg

@Silverteal
Copy link
Contributor Author

Silverteal commented Jun 14, 2024

那是异常的仓库,我这边就很正常,这种错误不应该为他们擦屁股,这通常是他们自己导致的。

异常的仓库应该是您的揣测。事实上的原因您刚才提到了,是因为当前分支没有可用的标签。这对新分支来说是不可避免的。不可能期待一个仓库刚刚完成复刻就立刻打标签,这不符合标签的用途。

[图片]

不知道您想要说明什么。

@allMagicNB
Copy link
Contributor

allMagicNB commented Jun 15, 2024

那是异常的仓库,我这边就很正常,这种错误不应该为他们擦屁股,这通常是他们自己导致的。

异常的仓库应该是您的揣测。事实上的原因您刚才提到了,是因为当前分支没有可用的标签。这对新分支来说是不可避免的。不可能期待一个仓库刚刚完成复刻就立刻打标签,这不符合标签的用途。

但是为什么 Branch 不能同步 Tag?

[图片]

不知道您想要说明什么。

常见吗?PCL 的任何发布版压缩包都没有包含任何的 PCL 字样

uses: actions/upload-artifact@v4
with:
name: ${{ matrix.configuration }} ${{ env.Describe }}
# 文件名格式示例:pcl2-autobuild-2.7.4-37-a0b1c2d3-Snapshot.zip
Copy link
Contributor

Choose a reason for hiding this comment

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

我还是那句话,你要不要先看看再说?

@WForst-Breeze
Copy link
Collaborator

WForst-Breeze commented Jun 15, 2024

提交的内容有一些问题是 不可避免 的,我们要做的是一起完善它。我也不是很懂 Actions,对内容就暂不做评价和建议了。但是就沟通问题上,我个人还是 恳请 诸位在沟通时语气上保证 最基本的平和和尊重

@allMagicNB
Copy link
Contributor

allMagicNB commented Jun 15, 2024

因此,我反对此 PR。

@Silverteal
Copy link
Contributor Author

Silverteal commented Jun 15, 2024

Working on this makes me tired. So I'm not adding any further enhancements to this any more, anyone with adjustments can make pull request to my branch, or ignore it until this pr is closed.

@Silverteal
Copy link
Contributor Author

Btw, I'd have thought that the developer had mentioned something like the "adding comments back" in #4009. My misunderstanding maybe though.

@LTCatt
Copy link
Member

LTCatt commented Jun 15, 2024

关于注释:
我在 #4009 提出加回注释,是因为既然已经有了注释,那就没有正向的理由把已有的注释又给删了,这是对前人工作的不尊重。但如果 #4009 是对代码的整体重构,这也是可以接受的。

关于争议:
以我个人视角看来,对这个文件的编辑已经从实现构建功能,演变成了一场 “正确性辩论”。
争议中的问题包括 “是否应该有注释”、“注释用什么语言”、“文件怎么命名”……但这些点实际上不会对功能造成任何影响,而是演变成了 类似于 “大括号要不要换行” 的正确性辩论 ,这是没有意义的,这类问题是无法得出让双方都满意的答案的。

个人意见:
鉴于这件事已经有演变成 编辑战 的势头,我会以以下标准减少 PR 数量:

  1. 不采纳 “正确性辩论” 类更改:例如增删注释、修改注释语言、修改文件命名
  2. 不采纳对没有出现过的问题的预防性更改
  3. 如果更改是中性的(没有明确地让功能更好,不过也没有更坏),则不采纳

就我个人意见而言,@allMagicNB@Silverteal 两人之间在这方面的观点存在冲突,应当先求同存异。如果继续 编辑战,不排除采取进一步措施的可能。

关于本 PR:
基于上述三个准则,本 PR 中除了 Line 51-56 的内容属于确实出现过的问题,可以保留,其余部分均应当回滚。
在回滚其他更改,保留 Line 51-56 的内容后会进行合并。

@allMagicNB
Copy link
Contributor

先别 Merge,我有个更优的解决方式,等会发个 PR(@LTCatt

@Silverteal
Copy link
Contributor Author

Silverteal commented Jun 15, 2024

这确实是编辑战。因此为了避免进一步的困扰,我暂时不进行更多评论。

我将继续保留最后一个用英语书写的声明,即不再主动更新此分支。任何有意向继续改进的人可以向此分支提交 PR ,或者忽略这个 PR 直到它被合并或者关闭。

——6月18日更新——
对于这是否算是编辑战,我重新保留疑问。对于 Wiki ,内容不需要审核。而 PR 的合并是需要审核的。因此,在 PR 中进行讨论应该类比 Wiki 中的讨论页辩论,因而在合并之前,它不应该算编辑战。

@allMagicNB
Copy link
Contributor

关了吧,51~56 也没啥用,一个参数就能代替(

@Silverteal
Copy link
Contributor Author

我将不会再重复第三遍我的声明。

@LTCatt
Copy link
Member

LTCatt commented Jun 15, 2024

如果 PR 作者不打算只保留 Ln51-56 那我就 Close 了?

@LTCatt LTCatt closed this Jun 15, 2024
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.

6 participants