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

Add Chinese code platform gitee webhooks #147

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

li-clement
Copy link

No description provided.

@coveralls
Copy link

coveralls commented Apr 18, 2022

Coverage Status

Coverage decreased (-1.2%) to 87.04% when pulling 2755e3c on robekeane:gitee into 9c954e2 on go-playground:master.

@li-clement
Copy link
Author

It seems to be a lint-tool issue

gitee/payload.go Outdated
)

type CommentEventPayload struct {
Action *string `json:"action,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be sure, do we actually need use pointers everywhere?
Probably sometimes it make sense to have simple types as is. Or you should distinguish empty string and nil?

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, you can generally omit "omitempty" for "incoming" types. You will use full structure anyway. It's more make sense for instantiating JSONs.

Copy link
Author

Choose a reason for hiding this comment

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

BTW, you can generally omit "omitempty" for "incoming" types. You will use full structure anyway. It's more make sense for instantiating JSONs.

Thanks, I'll make some improvements and then retest it.

gitee/payload.go Outdated
Before *string `json:"before,omitempty"`
After *string `json:"after,omitempty"`
TotalCommitsCount int64 `json:"total_commits_count,omitempty"`
CommitsMoreThanTen *bool `json:"commits_more_than_ten,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

The same thing about bool! Do we actually need to distinguish false and nil everywhere?

gitee/payload.go Outdated

// LabelHook : Label, issue and pull request labels
type LabelHook struct {
Id int32 `json:"id,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting thing! Sometimes you use int32, but sometimes int64. Does it make sense to have different types for IDs?

@Toshik1978
Copy link
Contributor

It seems to be a lint-tool issue

It should be good to investigate it. May be run linter locally. Are you sure, that linter pass successful?

@li-clement
Copy link
Author

li-clement commented Apr 22, 2022

The lint tool shows this, I don't know if there's something wrong with it. I test all the code on my own server, all the check items are pass.

level=error msg="Running error: buildir: failed to load package goarch: could not load export data: cannot import \"internal/goarch\" (unknown iexport format version 2), export data is newer version - update tool"

@Toshik1978
Copy link
Contributor

The lint tool shows this, I don't know if there's something wrong with it.

I'm a little bit busy right now, but will be able to investigate it in 1-2 weeks. Can you try, pls, the following: update linter version to the latest one (1.45.2). It's here:
https://github.com/go-playground/webhooks/blob/master/.github/workflows/workflow.yml#L19

@li-clement
Copy link
Author

The lint tool shows this, I don't know if there's something wrong with it.

I'm a little bit busy right now, but will be able to investigate it in 1-2 weeks. Can you try, pls, the following: update linter version to the latest one (1.45.2). It's here: https://github.com/go-playground/webhooks/blob/master/.github/workflows/workflow.yml#L19

@Toshik1978 Thanks for remind me that. I will try to fix it.

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