-
Notifications
You must be signed in to change notification settings - Fork 15
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
fix: hide disabled entry #308
Conversation
/assign @nighca |
user, | ||
accountList: [ | ||
const accountList = [] | ||
let temp = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: temp
这个变量似乎没用到,考虑
const accountList = []
;[/* ... */].forEach((e, i) => {
// ...
})
或者
const accountList = []
let temp = [/* ... */]
temp.forEach((e, i) => {
// ...
})
或者
const accountList = [/* ... */].filter(
// filter with provider
).map(
// get url
)
cmd/gopcomm/gop_autogen.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
改 HTML 也会生成新的 cmd/gopcomm/gop_autogen.go
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我们的go+版本不一样,所以通过gogen生成的代码不一样就导致了该问题。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
通过gogen生成的代码不一样
即使是这样也不应该在这个 PR 里提交这个变更?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这会很麻烦,它属于go+插件触发的自动生成行为,我将它的变更拆分到另一个pr中一样,会导致插件再生成一个新的
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这会很麻烦,它属于go+插件触发的自动生成行为
不要 git add 进来,就不会带进 commit 了
我将它的变更拆分到另一个pr中一样,会导致插件再生成一个新的
既然是 go+ 版本不同导致的结果文件变更,那么只在“go+ 版本变更”是当前 PR 预期包含的变更时才去提交这个结果
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
那么只在“go+ 版本变更”是当前 PR 预期包含的变更时才去提交这个结果
首先你们应该约定项目使用的 go+ 版本;那么可能的情况有两种:
- 你本地的 go+ 版本是约定的版本,而项目本来提交的不是,所以产生了变更
- 你本地的 go+ 版本不是约定的版本,而项目本来的是,所以产生了变更
如果是 2,变更就不是预期的,你应该升级本地版本;如果是 1,最好专门提一个 PR 做“升级项目 go+ 版本为约定版本”的事情,这个 PR 里提交 autogen 的变更,并在某个地方(比如 README)说明项目约定的 go+ 版本
ab01441
to
ccf29e7
Compare
ccf29e7
to
63ff258
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
应该是因为 format,整个文件都变更了(这个是应该避免或者至少拆 commit 的..),只看了相关的部分
63ff258
to
59f4e18
Compare
The PR environment is ready, please check the PR environment [Attention]: This environment will be automatically cleaned up after 6 hours, please make sure to test it in time. If you have any questions, please contact the author of the PR or the community team. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.