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

Simplify the code using struct #4

Merged
merged 4 commits into from
Aug 22, 2016

Conversation

adrianlzt
Copy link
Contributor

Now payload is a Struct, it's not needed to json encode.
Unfurling works with images.
Only asks for the token, webhook url is in the code.

Now payload is a Struct, it's not needed to json encode.
Unfurling works with images.
@@ -29,50 +27,33 @@ type Attachment struct {
Fields []*Field `json:"fields"`
}

type Payload struct {
Parse string `json:"parse,omitempty"`
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please format this with go fmt?

@ashwanthkumar
Copy link
Owner

Except for that one comment - the code doesn't seem to be formatted using go fmt, rest LGTM. I would probably create a release tag with the current HEAD since this is going to be a breaking change.

func Send(webhookUrl string, proxy string, payload map[string]interface{}) []error {
data, _ := json.Marshal(payload)
func Send(token string, proxy string, payload Payload) []error {
webhookUrl := fmt.Sprintf("https://hooks.slack.com/services/%v", token)
Copy link
Owner

@ashwanthkumar ashwanthkumar Aug 22, 2016

Choose a reason for hiding this comment

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

@adrianlzt Just happen to see this, sorry :( Can we please stick with full webhook url instead of just token? That way we can support Slack API Compatible services (like Rocket.Chat, etc.) as well.

@ashwanthkumar
Copy link
Owner

Thank you so much for the contribution.

@ashwanthkumar ashwanthkumar merged commit 6174793 into ashwanthkumar:master Aug 22, 2016
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.

2 participants